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

[Julien Grall <julien@xxxxxxx>]

Hi Juergen,

On 28/07/2023 14:24, Juergen Gross wrote:
> On 28.07.23 14:48, Julien Grall wrote:
> > Hi,
> >
> > On 28/07/2023 13:06, Juergen Gross wrote:
> > > On 28.07.23 13:19, Julien Grall wrote:
> > > > > > > In case of a runtime check I
> > > > > > > agree that a more central place would be preferred.
> > > > > > >
> > > > > > > In the end I don't mind that much, but
> > > > > > >
> > > > > > >      BUILD_BUG_ON(XENSTORE_PAYLOAD_MAX >=
> > > > > > >               (typeof((struct node_hdr *)NULL->datalen))(-1));
> > > > > > >
> > > > > > > is a little bit clumsy IMHO.
> > > > > > Agree. We could introduce FIELD_SIZEOF() (as Linux did) to hide 
> > > > > > the complexity. The code would then look like:
> > > > > >  >= (8 * FIELD_SIZEOF(struct node_hdr, datalen))
> > > > > Oh, I guess you mean sizeof_field().
> > > > >
> > > > > And even with that it would look quite clumsy:
> > > > >
> > > > >      BUILD_BUG_ON(XENSTORE_PAYLOAD_MAX >=
> > > > >               (1UL << (8 * sizeof_field(struct node_hdr, datalen))));
> > > > How about keeping the BUILD_BUG_ON() in write_node_raw() and add the 
> > > > following comment on top of handle_input():
> > > > Some fields in Xenstored are sized based on the max payload (see 
> > > > various BUILD_BUG_ON()). This would need extra runtime check if we 
> > > > ever decide to have a dynamic payload size.
> > > I _could_ do that, but where to stop adding such comments?
> > When someone other than the author is able to understand the code 
> > without too much effort. More comments never hurts, less will in the 
> > longer run (see below).
> I agree with that statement in general, but requesting a comment to aid a
> future potential change violating the Xenstore wire protocol is a little 
> bit
> weird.
Well... This is violating the existing protocol, but it is not set in 
stone and I think this is acceptable to update it when there is no 
change for the VMs and for new features (e.g. Live-Update/Live-Migration).
> > > TBH, I really don't see the point doing that.
> > >
> > > In case a patch came up upstream trying to violate 
> > > XENSTORE_PAYLOAD_MAX I would
> > > surely NACK it.
> > That's assuming you will still be around when this happens :). I am 
> > not wishing anything bad but the code will likely outlast any of us.
> Maybe. But would you really Ack patches adding comments like that in other
> areas?
Potentially yes. We had a similar discussion on Arm when allowing 
paddr_t to be 32-bit.
[...]

> > > In case we need payloads larger than XENSTORE_PAYLOAD_MAX we should 
> > > split the
> > > related operation in multiple parts (see e.g. XS_DIRECTORY_PART or 
> > > XS_CONTROL
> > > for uploading a new kernel to Xenstore-stubdom for live update). 
> > > Which is, BTW,
> > > the way AWS should have handled the migration problem (transactions 
> > > come to my
> > > mind in this context).
> > I wasn't part of the original design, but I can see why it was done 
> > like that.
> I can see why it was done that way, but this doesn't mean I can understand
> why such a design should be supported by adding comments helping to 
> repeat such
> a bad decision.
>
> > 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.
> 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())."
--
Julien Grall