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

[Juergen Gross <jgross@xxxxxxxx>]

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.

> > 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? E.g. cases where the domid is stored in an unsigned int and me adding
a comment to be careful in case domids ever grow to 64 bits?

> So we need to make easy for a future maintainers/reviewers to know the 
> assumptions and implications of changing some of the limits.
Yes, that's what the BUILD_BUG_ON() is meant for.

> > 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. :-)

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

> So one command as the advantage to simply a lot the logic in Xenstored.
Simplifying the logic while violating the basic wire protocol is a bad design
decision (I'm repeating myself, I know).

> Anyway, this is getting a bit off topic. My only request is to write down 
> assumption more explicitly rather than hiding them. A comment on top of the 
> check is a nice way to help the developper to avoid making a "bad" decision.
I have added a brief comment why the check is existing at all. I even managed
to avoid any strong language. :-)

        /* Some downstreams modify XENSTORE_PAYLOAD_MAX. */

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.


Juergen

Attachment: OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key

Attachment: OpenPGP_signature
Description: OpenPGP digital signature