Re: [PATCH v3 17/25] tools/xenstore: rework struct xs_tdb_record_hdr

[Julien Grall <julien@xxxxxxx>]

Hi Juergen,

On 28/07/2023 15:32, Juergen Gross wrote:
> On 28.07.23 16:08, Julien Grall wrote: >>>> Using multiple commands has also 
> its downside. The first that comes
> > > > to my mind if that you need to keep around the data. But, with your 
> > > > proposal, you we wouldn't be able to store it in the database (like 
> > > > for transaction update) as datalen can only be 65KB.
> > > I wasn't aware that a complete transaction needs to be kept in a 
> > > single data
> > > base record. :-)
> > IIUC, you are thinking that the client will restore all the state bits 
> > by bits. But if you look at the design in 
> > docs/designs/xenstore-migration.md, this is a blob.
> Of course it is.
>
> I was never assuming that the state would be applied piecemeal, this has to
> happen atomically.
I am confused because I don't see how this related to the discussion. 
Above, you mention a transaction, which I interpreted as the client 
would open a transaction and do a bunch of "write note", "set 
permissions"... And then commit the transaction.
I thought this is what you talked about and this would still be 
atomically. My point with the blob is that the parsing of the state is 
done by Xenstored, not the client.
> > > It would work perfectly fine to allocate the needed memory via 
> > > talloc() and to
> > > reference it from a special node being part of the transaction, or to 
> > > not use
> > > a node at all (see again the XS_CONTROL example).
> > I am not convinced the complexity is worth it here. To be honest, I 
> > think the payload limit should have been relaxed for Live-Update as 
> > well as you don't gain much to split. That said, this is less a 
> > concern because you are not time constrained.
> > [...]
> >
> > > But maybe that comment was based on wrong assumptions, like the 
> > > mentioned
> > > change not violating the protocol. >
> > > > I am happy to rewrite the comment so it doesn't lead to think that 
> > > > you (as the maintainer) are open to have a more relax length check.
> > > Yes, please make a suggestion for a proper comment not suggesting we 
> > > are fine
> > > to violate the wire protocol.
> > Here we go:
> >
> > "The payload size is not only currently restricted by the protocol but 
> > also the internal implementation (see various BUILD_BUG_ON())."
> Hmm, I'm still feeling uneasy to imply that the payload size might be 
> changed.
> See above reasoning.
>
> The only way I could imagine this being possible would be a per-ring-page
> attribute with both sides agreeing to the max allowed size (the minimum 
> being
> today's value).
>
> With that in mind I can hesitantly add the comment, maybe with the 
> addition:
> "Any potential change of the maximum payload size needs to be negotiated 
> between
> the involved parties."
I am ok with that.

Cheers,

--
Julien Grall