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

[Julien Grall <julien@xxxxxxx>]

On 17/03/2022 11:07, Julien Grall wrote:
> Hi Juergen,
>
> 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.
> This is not new, so not change expected in this series.
>
> >   -----------------------------------------------------------------
> >   1       Ring reconnection (see the ring reconnection feature below)
> >   2       Connection error indicator (see connection error feature below)
> > +4       GET_FEATURE and SET_FEATURE Xenstore wire commands are available
> Below, you wrote the two commands are dom0 only. Furthermore, I would 
> expect such comment return something like ENOSYS if they are not present.
> So do we really need to add a feature bit for GET_FEATURE/SET_FEATURE?
>
> >   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>?
Please ignore this question. <value>  is part of the response. Sorry for 
the noise.
Cheers,

> > +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?
> Cheers,
--
Julien Grall