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

[Julien Grall <julien@xxxxxxx>]

Hi Juergen,

On 28/07/2023 10:14, Juergen Gross wrote:
> On 28.07.23 10:59, Julien Grall wrote:
> > Hi Juergen,
> >
> > On 28/07/2023 07:23, Juergen Gross wrote:
> > > On 27.07.23 23:53, Julien Grall wrote:
> > > > Hi Juergen,
> > > >
> > > > On 24/07/2023 12:02, Juergen Gross wrote:
> > > > > Struct xs_tdb_record_hdr is used for nodes stored in the data base.
> > > > > When working on a node, struct node is being used, which is including
> > > > > the same information as struct xs_tdb_record_hdr, but in a different
> > > > > format. Rework struct xs_tdb_record_hdr in order to prepare including
> > > > > it in struct node.
> > > > >
> > > > > Do the following modifications:
> > > > >
> > > > > - move its definition to xenstored_core.h, as the reason to put it 
> > > > > into
> > > > >    utils.h are no longer existing
> > > > >
> > > > > - rename it to struct node_hdr, as the "tdb" in its name has only
> > > > >    historical reasons
> > > > >
> > > > > - replace the empty permission array at the end with a comment about
> > > > >    the layout of data in the data base (concatenation of header,
> > > > >    permissions, node contents, and children list)
> > > > >
> > > > > - use narrower types for num_perms and datalen, as those are naturally
> > > > >    limited to XENSTORE_PAYLOAD_MAX (childlen is different here, as 
> > > > > it is
> > > > >    in theory basically unlimited)
> > > > The assumption is XENSTORE_PAYLOAD_MAX will never change and/or 
> > > > always apply for all the connection. I am aware of at least one 
> > > > downstream use where this is not the case.
> > > > I am happy to use narrower types, but I would at least like some 
> > > > checks in Xenstore to ensure the limits are not reached.
> > > I will add a BUILD_BUG_ON().
> > Initially I was thinking about a runtime check, but I am also fine 
> > with a BUILD_BUG_ON() if it is right next to length check in 
> > handle_input(). So if someone decided to add different payload size 
> > depending on the domain (such as privileged domain could do more), it 
> > would be easier to spot what else needs to be changed.
> Is this really the correct placement?
I think so. By adding the BUILD_ON_BUG() right above the length check, 
it would be easier for everyone to know that the datastructure may also 
need to change. This ...
> I've put it into write_node_raw(), as this is where the related datalen 
> member
> is being filled. 
... would be less obvious here and I think it might be miss.

Cheers,

--
Julien Grall