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

[Juergen Gross <jgross@xxxxxxxx>]

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've put it into write_node_raw(), as this is where the related datalen member
is being filled. This placement has the further advantage that I already have
a related pointer at hand, so I can use:

        BUILD_BUG_ON(XENSTORE_PAYLOAD_MAX >= (typeof(hdr->datalen))(-1));

which is exactly what should be tested.

> > > OOI, do you have a use case where a node would be shared with more than 255 
> > > domains?
> > No, but limiting it wouldn't give any real advantage.
> I guess by advantage you mean that the size of the structure would still be the 
> same. I thought this was the rationale but I asked just in case you had 
> something else in mind. For instance, Xen technically supports up to ~32 000 
> domains. But I think it would be crazy to decide to have more than a few tens 
> permissions on a node :).
Yes to both. OTOH why should we deny to allow that without any urgent need to do
so?


Juergen

Attachment: OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key

Attachment: OpenPGP_signature
Description: OpenPGP digital signature