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

[Juergen Gross <jgross@xxxxxxxx>]

On 17.03.22 12: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.
Maybe this is in order to avoid big/little endian issues (which bit is
bit 0?)

> 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?
Good question. I'd be fine to drop it.

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


Juergen

Attachment: OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key

Attachment: OpenPGP_signature
Description: OpenPGP digital signature