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

[Julien Grall <julien@xxxxxxx>]

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).
> 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.
So we need to make easy for a future maintainers/reviewers to know the 
assumptions and implications of changing some of the limits.
> 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.
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.
So one command as the advantage to simply a lot the logic in Xenstored.

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 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.
Cheers,

--
Julien Grall