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

[Julien Grall <julien@xxxxxxx>]

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

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