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

[Juergen Gross <jgross@xxxxxxxx>]

On 18.03.22 19:40, Julien Grall wrote:
> 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).
Fine with me.


Juergen

Attachment: OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key

Attachment: OpenPGP_signature
Description: OpenPGP digital signature