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

[Juergen Gross <jgross@xxxxxxxx>]

On 28.07.23 16:59, Julien Grall wrote:
> 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.
My idea was that the transaction would be used to mark the related sub-commands
to belong to a single migration. This would make cleaning up in case of client
death much simpler.

Not using transactions would be possible, too, of course.

> > > > 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.
Okay.


Juergen

Attachment: OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key

Attachment: OpenPGP_signature
Description: OpenPGP digital signature