Re: [PATCH 1/3] tools/xenstore: add documentation for new set/get-feature commands

[Julien Grall <julien@xxxxxxx>]

Hi Juergen,

On 17/03/2022 11:19, Juergen Gross wrote:
> On 17.03.22 12:07, Julien Grall wrote:
> > On 16/03/2022 16:10, Juergen Gross wrote:
> > > Add documentation for two new Xenstore wire commands SET_FEATURE and
> > > GET_FEATURE used to set or query the Xenstore features visible in the
> > > ring page of a given domain.
> > >
> > > Signed-off-by: Juergen Gross <jgross@xxxxxxxx>
> > > ---
> > >   docs/misc/xenstore-ring.txt |  1 +
> > >   docs/misc/xenstore.txt      | 12 ++++++++++++
> > >   2 files changed, 13 insertions(+)
> > >
> > > diff --git a/docs/misc/xenstore-ring.txt b/docs/misc/xenstore-ring.txt
> > > index f91accb5b0..bd000f694e 100644
> > > --- a/docs/misc/xenstore-ring.txt
> > > +++ b/docs/misc/xenstore-ring.txt
> > > @@ -68,6 +68,7 @@ Mask    Description
> > I find a bit odd we describe the feature in term of mask rather bit. 
> > This will get more difficult to read as we add more bits.
> Maybe this is in order to avoid big/little endian issues (which bit is
> bit 0?)
Both end have to talk the same endianess. Otherwise, one may read the 
wrong value.
So long they are using the same endianess, bit 0 is not going to be matter.

> > >   The "Connection state" field is used to request a ring close and 
> > > reconnect.
> > >   The "Connection state" field only contains valid data if the server 
> > > has
> > > diff --git a/docs/misc/xenstore.txt b/docs/misc/xenstore.txt
> > > index ea3d8be177..31e3d53c52 100644
> > > --- a/docs/misc/xenstore.txt
> > > +++ b/docs/misc/xenstore.txt
> > > @@ -332,6 +332,18 @@ SET_TARGET        <domid>|<tdomid>|
> > >       xenstored prevents the use of SET_TARGET other than by dom0.
> > > +GET_FEATURE        <domid>|        <value>|
> > Did you indented to add many spaces before <value>?
> >
> > > +SET_FEATURE        <domid>|<value>|
> > > +    Returns or sets the contents of the "feature" field located at
> > > +    offset 2064 of the Xenstore ring page of the domain specified by
> > > +    <domid>. <value> is a decimal number being a logical or of the
> > > +    feature bits as defined in docs/misc/xenstore-ring.txt. Trying
> > > +    to set a bit for a feature not being supported by the running
> > > +    Xenstore will be denied.
> > How will the caller know which feature is supported? Also, what happen 
> > if a client decided to overwrite 'feature'? Could the result 
> > potentially prevent migration/liveupdate or else?
> The caller could use "GET_FEATURE 0" to see the available features, 
> assuming
> that nobody would have changed dom0's features.
>
> I'm not sure whether we should prevent dom0's features to be overwritten.
I think it would be better to have a separate "domid" (maybe "server" or 
"global") to retrieve features supported by the server.
This would give us some flexibility to update dom0 features in the 
future if the needs arise (the first implementation may forbid it).
Cheers,

--
Julien Grall