[PATCH v3 0/4] docs: Document xenstore paths

[PATCH v3 0/4] docs: Document xenstore paths

[Paul Durrant]

Patch #1 documents paths, some already in used by XenServer, which can be
used by guests to advertise contol capabilities.

Patch #2 documents paths which can be used to advertise PV driver versions.

Patch #3 documents paths which can be used by guests to advertise hotplug
capabilities.

Patch #4 documents paths which can be used by guests to advertise network
addresses.

[PATCH v3 1/4] docs: Document control features the can be advertised by guests...

[Paul Durrant]

...via xenstore.

XenServer already makes use of ~/control/feature-suspend being written
to advertise guest capability of responding to 'suspend' when written to
~/control/shutdown and, since they are derived from XenServer drivers,
the Xen Project Windows PV drivers attempt to write this value. The write
currently fails for libxl provisioned VMs because ~/control is read-only
to the guest (only ~/control/shutdown is writable, for ackowledgement
purposes).

This patch documents feature-suspend and also a set of similar control
feature flags, so that that they may be added to libxl provisioned
guests by subsequent patches:

feature-halt: PV drivers/agent can halt the guest
feature-reboot: PV drivers/agent can reboot the guest
feature-s3: PV drivers/agent can trigger guest sleep (HVM only)
feature-s4: PV drivers/agent can trigger guest hibernate (HVM only)

The patch (bacause it adds features relating to S3 and S4 power states)
also clarifies that the initial set of platform properties mentioned are
booleans, and updates the specifier accordingly.

Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
Cc: Ian Campbell <ian.campbell@citrix.com>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Keir Fraser <keir@xen.org>
Cc: Tim Deegan <tim@xen.org>
---

v3:
 - Changed feature-halt to feature-poweroff since writing 'poweroff' to
   control/shutdown is the currently implemented method of PV domain
   shutdown
---
 docs/misc/xenstore-paths.markdown | 38 +++++++++++++++++++++++++++++++++++---
 1 file changed, 35 insertions(+), 3 deletions(-)

diff --git a/docs/misc/xenstore-paths.markdown b/docs/misc/xenstore-paths.markdown
index d94ea9d..71dfda2 100644
--- a/docs/misc/xenstore-paths.markdown
+++ b/docs/misc/xenstore-paths.markdown
@@ -178,9 +178,9 @@ will not relocate guest memory.
 
 The BIOS used by this domain.
 
-#### ~/platform/* [HVM,INTERNAL]
+#### ~/platform/* = ("0"|"1") [HVM,INTERNAL]
 
-Various platform properties.
+Various boolean platform properties.
 
 * acpi -- is ACPI enabled for this domain
 * acpi_s3 -- is ACPI S3 support enabled for this domain
@@ -321,12 +321,44 @@ by udev ("0") or will be run by the toolstack directly ("1").
 #### ~/control/shutdown = (""|COMMAND) [w]
 
 This is the PV shutdown control node. A toolstack can write various
-commands here to cause various guest shutdown, reboot or suspend
+commands here to cause various guest halt, reboot or suspend
 activities. The guest acknowledges a request by writing the empty
 string back to the command node.
 
 The precise protocol is not yet documented.
 
+#### ~/control/feature-poweroff = (""|"0"|"1") [w]
+
+This is initialized to "" by the toolstack but may be set to 0 or 1 by
+a guest to indicate that it is capable or incapable, respectively, of
+shutting down if 'poweroff' is written to ~/control/shutdown.
+
+#### ~/control/feature-reboot = (""|"0"|"1") [w]
+
+This is initialized to "" by the toolstack but may be set to 0 or 1 by
+a guest to indicate that it is capable or incapable, respectively, of
+rebooting if 'reboot' is written to ~/control/shutdown.
+
+#### ~/control/feature-suspend = (""|"0"|"1") [w]
+
+This is initialized to "" by the toolstack but may be set to 0 or 1 by
+a guest to indicate that it is capable or incapable, respectively, of
+suspending if 'suspend' is written to ~/control/shutdown.
+
+#### ~/control/feature-s3 = (""|"0"|"1") [w,HVM]
+
+This is initialized to "" by the toolstack but may be set to 0 or 1 by
+a guest to indicate that it is capable or incapable, respectively, of
+triggering a guest sleep if 's3' is written to ~/control/shutdown and
+~/platform/acpi_s3 is 1.
+
+#### ~/control/feature-s4 = (""|"0"|"1") [w,HVM]
+
+This is initialized to "" by the toolstack but may be set to 0 or 1 by
+a guest to indicate that it is capable or incapable, respectively, of
+triggering a guest hibernate if 's4' is written to ~/control/shutdown
+and ~/platform/acpi_s4 is 1.
+
 #### ~/control/platform-feature-multiprocessor-suspend = (0|1) []
 
 Indicates to the guest that this platform supports the multiprocessor
-- 
2.1.4

[PATCH v3 2/4] docs: Document a xenstore path for PV driver version information...

[Paul Durrant]

...to be advertised by a guest.

For domain management purposes it is convenient to be able to see
PV driver version information in xenstore. The XAPI toolstack in
XenServer has always created a ~/drivers path for this purpose.

This patch documents that path and also adds a specification of how
it should be used.

Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
Cc: Ian Campbell <ian.campbell@citrix.com>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Keir Fraser <keir@xen.org>
Cc: Tim Deegan <tim@xen.org>
---

v2:
 - Modify version specifier to allow for single-part version numbers
   and also arbitrary string suffix (e.g. '-debug')
---
 docs/misc/xenstore-paths.markdown | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/docs/misc/xenstore-paths.markdown b/docs/misc/xenstore-paths.markdown
index 71dfda2..cae5115 100644
--- a/docs/misc/xenstore-paths.markdown
+++ b/docs/misc/xenstore-paths.markdown
@@ -37,6 +37,8 @@ before regexp expansion:
   the "other" domain. i.e. ~ refers to the domain providing a service
   while $DOMID is the consumer of that service.
 * $UUID -- a UUID in the form xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx
+* $NAME -- a string identifying an object within a domain. Typically
+  comprising only alphanumeric characters.
 
 VALUES are strings and can take the following forms:
 
@@ -51,6 +53,11 @@ VALUES are strings and can take the following forms:
 * (VALUE | VALUE | ... ) -- a set of alternatives. Alternatives are
   separated by a "|" and all the alternatives are enclosed in "(" and
   ")".
+* VERSION -- 1, 2 or 3 integers, in decimal form separated by "."
+             followed by a STRING suffix (starting with the first
+             non-numeric character).
+             The integers are a version number of the form MAJOR,
+             MAJOR.MINOR or MAJOR.MINOR.MICRO respectively.
 
 Additional TAGS may follow as a comma separated set of the following
 tags enclosed in square brackets.
@@ -377,6 +384,11 @@ protocol definition.
 
 A domain writable path. Available for arbitrary domain use.
 
+#### ~/drivers/$NAME/version = VERSION [w]
+
+A domain may write version information for PV driver $NAME using
+this path.
+
 ### Paths private to the toolstack
 
 #### ~/device-model/$DOMID/state [w]
-- 
2.1.4

Re: [PATCH v3 2/4] docs: Document a xenstore path for PV driver version information...

[Ian Jackson]

Paul Durrant writes ("[Xen-devel] [PATCH v3 2/4] docs: Document a xenstore path for PV driver version information..."):
> diff --git a/docs/misc/xenstore-paths.markdown b/docs/misc/xenstore-paths.markdown
> index 71dfda2..cae5115 100644
> --- a/docs/misc/xenstore-paths.markdown
> +++ b/docs/misc/xenstore-paths.markdown
> @@ -37,6 +37,8 @@ before regexp expansion:
>    the "other" domain. i.e. ~ refers to the domain providing a service
>    while $DOMID is the consumer of that service.
>  * $UUID -- a UUID in the form xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx
> +* $NAME -- a string identifying an object within a domain. Typically
> +  comprising only alphanumeric characters.
...
> +* VERSION -- 1, 2 or 3 integers, in decimal form separated by "."
> +             followed by a STRING suffix (starting with the first
> +             non-numeric character).
> +             The integers are a version number of the form MAJOR,
> +             MAJOR.MINOR or MAJOR.MINOR.MICRO respectively.

This string is not really useful or useable in this form.  Who assigns
these version numbers ?  Maybe we need Vendor / Product / Version or
something (a la SCSI or something).

Also we need some explanation of what this is for.

In particular, is it part of protocol negotiation or bug workaround ?
Given the format, hopefully not.  In which case that needs to be
explicitly stated.

If it is to be used for bug workaround, it definitely needs to be more
structured.

Thanks,
Ian.

Re: [PATCH v3 2/4] docs: Document a xenstore path for PV driver version information...

[Paul Durrant]

> -----Original Message-----
> From: Ian Jackson [mailto:Ian.Jackson@eu.citrix.com]
> Sent: 13 November 2015 16:12
> To: Paul Durrant
> Cc: xen-devel@lists.xenproject.org; Keir (Xen.org); Ian Campbell; Tim
> (Xen.org); Jan Beulich
> Subject: Re: [Xen-devel] [PATCH v3 2/4] docs: Document a xenstore path for
> PV driver version information...
> 
> Paul Durrant writes ("[Xen-devel] [PATCH v3 2/4] docs: Document a xenstore
> path for PV driver version information..."):
> > diff --git a/docs/misc/xenstore-paths.markdown b/docs/misc/xenstore-
> paths.markdown
> > index 71dfda2..cae5115 100644
> > --- a/docs/misc/xenstore-paths.markdown
> > +++ b/docs/misc/xenstore-paths.markdown
> > @@ -37,6 +37,8 @@ before regexp expansion:
> >    the "other" domain. i.e. ~ refers to the domain providing a service
> >    while $DOMID is the consumer of that service.
> >  * $UUID -- a UUID in the form xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx
> > +* $NAME -- a string identifying an object within a domain. Typically
> > +  comprising only alphanumeric characters.
> ...
> > +* VERSION -- 1, 2 or 3 integers, in decimal form separated by "."
> > +             followed by a STRING suffix (starting with the first
> > +             non-numeric character).
> > +             The integers are a version number of the form MAJOR,
> > +             MAJOR.MINOR or MAJOR.MINOR.MICRO respectively.
> 
> This string is not really useful or useable in this form.  Who assigns
> these version numbers ?  Maybe we need Vendor / Product / Version or
> something (a la SCSI or something).
> 

I like that idea.

> Also we need some explanation of what this is for.
> 
> In particular, is it part of protocol negotiation or bug workaround ?
> Given the format, hopefully not.  In which case that needs to be
> explicitly stated.
> 
> If it is to be used for bug workaround, it definitely needs to be more
> structured.

No, that's not the intention. The intention is that the information is mainly for display purposes, but sufficiently structured for a UI/toolstack to say 'hey, your drivers are out of date'. I think having the vendor and product names in there help that.

  Paul

> 
> Thanks,
> Ian.

Re: [PATCH v3 2/4] docs: Document a xenstore path for PV driver version information...

[Ian Jackson]

Paul Durrant writes ("RE: [Xen-devel] [PATCH v3 2/4] docs: Document a xenstore path for PV	driver version information..."):
> [Ian J:]
> > If it is to be used for bug workaround, it definitely needs to be more
> > structured.
> 
> No, that's not the intention. The intention is that the information is mainly for display purposes, but sufficiently structured for a UI/toolstack to say 'hey, your drivers are out of date'. I think having the vendor and product names in there help that.

Right, good.

You should probably import (textually, perhaps) the version number
syntax from Debian, rather than inventing your own syntax of integer
tuples.  (It is implemented by GNU coreutils nowadays...)

Ian.

[PATCH v3 3/4] docs: Document xenstore paths for domain hotplug features

[Paul Durrant]

Without some indication from a domain it is not possible for a
toolstack to know whether instantiation of a new vbd or vif should
result in a new PV device of the appropriate type being instantiated
in a guest. (In other words whether PV drivers are present and
functioning).

This patch documents two paths which vif and vbd frontend drivers can
use to advertise their ability to respond to new vif or vbd
instantiations.

Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
Cc: Ian Campbell <ian.campbell@citrix.com>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Keir Fraser <keir@xen.org>
Cc: Tim Deegan <tim@xen.org>
---

v2:
 - Drop HVM restriction
---
 docs/misc/xenstore-paths.markdown | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/docs/misc/xenstore-paths.markdown b/docs/misc/xenstore-paths.markdown
index cae5115..6fecdf3 100644
--- a/docs/misc/xenstore-paths.markdown
+++ b/docs/misc/xenstore-paths.markdown
@@ -389,6 +389,18 @@ A domain writable path. Available for arbitrary domain use.
 A domain may write version information for PV driver $NAME using
 this path.
 
+#### ~/feature/hotplug/vif = ("0"|"1") [w]
+
+An HVM domain can indicate to a toolstack that it is capable
+of responding to instantiation of a new vif by bringing online a
+new PV network device without the need for a reboot.
+
+#### ~/feature/hotplug/vbd = ("0"|"1") [w]
+
+An HVM domain can indicate to a toolstack that it is capable
+of responding to instantiation of a new vbd by bringing online a
+new PV block device without the need for a reboot.
+
 ### Paths private to the toolstack
 
 #### ~/device-model/$DOMID/state [w]
-- 
2.1.4

[PATCH v3 4/4] docs: Document xenstore paths for domain network address information

[Paul Durrant]

It is useful to be able to see the network addresses in use by a domain
for a particular vif in xenstore, for example so that ssh can be invoked
by a toolstack to log into the guest domain.

This patch documents paths to allow a domain to advertise MAC (unicast
and multicast) and IP (versions 4 and 6) address information.

Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
Cc: Ian Campbell <ian.campbell@citrix.com>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Keir Fraser <keir@xen.org>
Cc: Tim Deegan <tim@xen.org>
---

v2:
 - Allow for compression of IPv6 addresses
---
 docs/misc/xenstore-paths.markdown | 30 ++++++++++++++++++++++++++++++
 1 file changed, 30 insertions(+)

diff --git a/docs/misc/xenstore-paths.markdown b/docs/misc/xenstore-paths.markdown
index 6fecdf3..cca29e6 100644
--- a/docs/misc/xenstore-paths.markdown
+++ b/docs/misc/xenstore-paths.markdown
@@ -39,6 +39,8 @@ before regexp expansion:
 * $UUID -- a UUID in the form xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx
 * $NAME -- a string identifying an object within a domain. Typically
   comprising only alphanumeric characters.
+* $INDEX -- an integer used as part of a path when listing a set of
+            values. Typically these integers are contiguous.
 
 VALUES are strings and can take the following forms:
 
@@ -58,6 +60,14 @@ VALUES are strings and can take the following forms:
              non-numeric character).
              The integers are a version number of the form MAJOR,
              MAJOR.MINOR or MAJOR.MINOR.MICRO respectively.
+* MAC_ADDRESS -- 6 integers, in hexadecimal form, separated by ':',
+                 specifying an ethernet MAC address.
+* IPV4_ADDRESS -- 4 integers, in decimal form, separated by '.',
+                  specifying an IP version 4 address.
+* IPV6_ADDRESS -- Up to 8 integers, in hexadecimal form, separated
+                  by ':', specifying an IP version 6 address.
+		  (Zero compression of addresses, using '::' notation,
+		  is allowed but not required).
 
 Additional TAGS may follow as a comma separated set of the following
 tags enclosed in square brackets.
@@ -401,6 +411,26 @@ An HVM domain can indicate to a toolstack that it is capable
 of responding to instantiation of a new vbd by bringing online a
 new PV block device without the need for a reboot.
 
+#### ~/attr/vif/$DEVID/name = STRING [w]
+
+A domain may write its internal 'friendly' name for a network device
+using this path.
+
+#### ~/attr/vif/$DEVID/mac/$INDEX = MAC_ADDRESS [w]
+
+A domain may write the set of MAC addresses (unicast and multicast)
+in use by the network frontend using paths of this form.
+
+#### ~/attr/vif/$DEVID/ipv4/$INDEX = IPV4_ADDRESS [w]
+
+A domain may write the set of IP version 4 addresses in use by the
+stack bound to the network frontend using paths of this form.
+
+#### ~/attr/vif/$DEVID/ipv6/$INDEX = IPV6_ADDRESS [w]
+
+A domain may write the set of IP version 6 addresses in use by the
+stack bound to the network frontend using paths of this form.
+
 ### Paths private to the toolstack
 
 #### ~/device-model/$DOMID/state [w]
-- 
2.1.4

Re: [PATCH v3 4/4] docs: Document xenstore paths for domain network address information

[Ian Jackson]

Paul Durrant writes ("[PATCH v3 4/4] docs: Document xenstore paths for domain network address information"):
> +#### ~/attr/vif/$DEVID/name = STRING [w]

You need to explicitly state what this information is to be used for.

This "name" is presumably useful for humans in the management UI ?
You need to say something about the toolstack not trusting it too
much.

> +A domain may write its internal 'friendly' name for a network device
> +using this path.
> +
> +#### ~/attr/vif/$DEVID/mac/$INDEX = MAC_ADDRESS [w]
> +
> +A domain may write the set of MAC addresses (unicast and multicast)
> +in use by the network frontend using paths of this form.

The primary (virtual hardware) VIF MAC address is already recorded by
the toolstack in the PV protocol backend directory.

What is this for ?

At the very least you need to state that this information MUST NOT be
used for packet routing, packet filtering, or for security purposes.

> +#### ~/attr/vif/$DEVID/ipv4/$INDEX = IPV4_ADDRESS [w]
> +#### ~/attr/vif/$DEVID/ipv6/$INDEX = IPV6_ADDRESS [w]

I can see that this would be easier to use than messing with querying
the DHCP server or whatever.

But you absolutely must state that this information MUST NOT be used
for packet routing etc.  Examples of what it _is_ good for would be
good.

Thanks,
Ian.

Re: [PATCH v3 4/4] docs: Document xenstore paths for domain network address information

[Paul Durrant]

> -----Original Message-----
> From: Ian Jackson [mailto:Ian.Jackson@eu.citrix.com]
> Sent: 13 November 2015 16:18
> To: Paul Durrant
> Cc: xen-devel@lists.xenproject.org; Ian Campbell; Jan Beulich; Keir (Xen.org);
> Tim (Xen.org)
> Subject: Re: [PATCH v3 4/4] docs: Document xenstore paths for domain
> network address information
> 
> Paul Durrant writes ("[PATCH v3 4/4] docs: Document xenstore paths for
> domain network address information"):
> > +#### ~/attr/vif/$DEVID/name = STRING [w]
> 
> You need to explicitly state what this information is to be used for.
> 
> This "name" is presumably useful for humans in the management UI ?
> You need to say something about the toolstack not trusting it too
> much.
> 
> > +A domain may write its internal 'friendly' name for a network device
> > +using this path.
> > +
> > +#### ~/attr/vif/$DEVID/mac/$INDEX = MAC_ADDRESS [w]
> > +
> > +A domain may write the set of MAC addresses (unicast and multicast)
> > +in use by the network frontend using paths of this form.
> 
> The primary (virtual hardware) VIF MAC address is already recorded by
> the toolstack in the PV protocol backend directory.
> 
> What is this for ?
> 

The frontend is free to ignore it and Windows has such override mechanisms. I view the address in the backend area as the virtual NIC's burnt-in address. This area is for the displaying all MAC addresses for which the frontend is processing traffic i.e. current unicast and all multicast addresses in operation. (No point in making broadcast address explicit).

> At the very least you need to state that this information MUST NOT be
> used for packet routing, packet filtering, or for security purposes.
> 

It is for information only. I'll call that out.

> > +#### ~/attr/vif/$DEVID/ipv4/$INDEX = IPV4_ADDRESS [w]
> > +#### ~/attr/vif/$DEVID/ipv6/$INDEX = IPV6_ADDRESS [w]
> 
> I can see that this would be easier to use than messing with querying
> the DHCP server or whatever.
> 
> But you absolutely must state that this information MUST NOT be used
> for packet routing etc.  Examples of what it _is_ good for would be
> good.
> 

Mostly for display purposes, but a UI could use it for kicking off a VNC or SSH session... I'll add that to the doc.

  Paul

> Thanks,
> Ian.

Re: [PATCH v3 4/4] docs: Document xenstore paths for domain network address information

[Ian Jackson]

Paul Durrant writes ("RE: [PATCH v3 4/4] docs: Document xenstore paths for domain network address information"):
> [Ian:]
> > The primary (virtual hardware) VIF MAC address is already recorded by
> > the toolstack in the PV protocol backend directory.
> > 
> > What is this for ?
> 
> The frontend is free to ignore it

Indeed.

> and Windows has such override mechanisms. I view the address in the
> backend area as the virtual NIC's burnt-in address.

Right.

> This area is for the displaying all MAC addresses for which the
> frontend is processing traffic i.e. current unicast and all
> multicast addresses in operation. (No point in making broadcast
> address explicit).

Right.

> > At the very least you need to state that this information MUST NOT be
> > used for packet routing, packet filtering, or for security purposes.
> 
> It is for information only. I'll call that out.

Right.

> Mostly for display purposes, but a UI could use it for kicking off a
> VNC or SSH session... I'll add that to the doc.

Right.  I think these are safe uses, since the guest controls this
value but if you are trying to ssh to the guest you want what the
guest thinks is its own IP address.

Ian.

Re: [PATCH v3 0/4] docs: Document xenstore paths

[Andrew Cooper]

On 10/11/15 17:40, Paul Durrant wrote:
> Patch #1 documents paths, some already in used by XenServer, which can be
> used by guests to advertise contol capabilities.
>
> Patch #2 documents paths which can be used to advertise PV driver versions.
>
> Patch #3 documents paths which can be used by guests to advertise hotplug
> capabilities.
>
> Patch #4 documents paths which can be used by guests to advertise network
> addresses.

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

Re: [PATCH v3 0/4] docs: Document xenstore paths

[Ian Jackson]

Paul Durrant writes ("[Xen-devel] [PATCH v3 0/4] docs: Document xenstore paths"):
> Patch #1 documents paths, some already in used by XenServer, which can be
> used by guests to advertise contol capabilities.

Thanks for these patches.  I am pleased to see what for upstream Xen
are new facilities and new protocol features, and to see you
upstreaming this XenServer work.

I think though that the docs patches need a slightly different
`angle', and are lacking some crucial information.

Firstly, the patch descriptions say `Document xenstore paths ...'
But from the point of view of upstream xen.git, these are new paths.
So I think the patch titles should be `Introduce xenstore paths for ...'

The documents need to acknowledge that the paths may not be supported
by either end.  So rather than "the toolstack will create", say "the
toolstack may create", for example.

And the documents themselves should then clearly state what the
behaviour should be on the part of a toolstack which finds the
information missing, or set to a particular values.

Likewise, they should document the expected behaviour of a guest which
finds that the entries are missing or cannot be written for
permissions reasons.

I have a few patch-specific comments too, which I will reply to in the
individual patches.

Thanks,
Ian.

Re: [PATCH v3 0/4] docs: Document xenstore paths

[Paul Durrant]

> -----Original Message-----
> From: Ian Jackson [mailto:Ian.Jackson@eu.citrix.com]
> Sent: 13 November 2015 16:10
> To: Paul Durrant
> Cc: xen-devel@lists.xenproject.org
> Subject: Re: [Xen-devel] [PATCH v3 0/4] docs: Document xenstore paths
> 
> Paul Durrant writes ("[Xen-devel] [PATCH v3 0/4] docs: Document xenstore
> paths"):
> > Patch #1 documents paths, some already in used by XenServer, which can
> be
> > used by guests to advertise contol capabilities.
> 
> Thanks for these patches.  I am pleased to see what for upstream Xen
> are new facilities and new protocol features, and to see you
> upstreaming this XenServer work.
> 
> I think though that the docs patches need a slightly different
> `angle', and are lacking some crucial information.
> 
> Firstly, the patch descriptions say `Document xenstore paths ...'
> But from the point of view of upstream xen.git, these are new paths.
> So I think the patch titles should be `Introduce xenstore paths for ...'
> 

Ok.

> The documents need to acknowledge that the paths may not be supported
> by either end.  So rather than "the toolstack will create", say "the
> toolstack may create", for example.
> 

Sure.

> And the documents themselves should then clearly state what the
> behaviour should be on the part of a toolstack which finds the
> information missing, or set to a particular values.
> 

Yes, I think that would be useful to add. In trying to implement the associated changed in libxl I can see it would be valuable :-)

> Likewise, they should document the expected behaviour of a guest which
> finds that the entries are missing or cannot be written for
> permissions reasons.
> 

In general from the guest PoV there's not a lot it can do if a path is not writable, but I'll call that out.

> I have a few patch-specific comments too, which I will reply to in the
> individual patches.
> 

Thanks,

  Paul

> Thanks,
> Ian.

Re: [PATCH v3 0/4] docs: Document xenstore paths

[Ian Jackson]

Paul Durrant writes ("RE: [Xen-devel] [PATCH v3 0/4] docs: Document xenstore paths"):
> [Ian:]
> > And the documents themselves should then clearly state what the
> > behaviour should be on the part of a toolstack which finds the
> > information missing, or set to a particular values.
> 
> Yes, I think that would be useful to add. In trying to implement the
> associated changed in libxl I can see it would be valuable :-)

Right.  In particular, your specification needs to be compatible with
the current implementation in libxl, as well as with a future
implementation which actually uses these new features :-).

> > Likewise, they should document the expected behaviour of a guest which
> > finds that the entries are missing or cannot be written for
> > permissions reasons.
> 
> In general from the guest PoV there's not a lot it can do if a path
> is not writable, but I'll call that out.

Right.  But presumably you want to say that the guest may (try to)
write them even if they don't already exist (that's right, isn't it?),
but that the guest MUST NOT fail just because it can't write them.

If it can't write them it should simply carry on (maybe making a note
in an obscure and non-alarming logfile somewhere).

thanks,
Ian.

Re: [PATCH v3 0/4] docs: Document xenstore paths

[Paul Durrant]

> -----Original Message-----
> From: Ian Jackson [mailto:Ian.Jackson@eu.citrix.com]
> Sent: 13 November 2015 16:20
> To: Paul Durrant
> Cc: xen-devel@lists.xenproject.org
> Subject: RE: [Xen-devel] [PATCH v3 0/4] docs: Document xenstore paths
> 
> Paul Durrant writes ("RE: [Xen-devel] [PATCH v3 0/4] docs: Document
> xenstore paths"):
> > [Ian:]
> > > And the documents themselves should then clearly state what the
> > > behaviour should be on the part of a toolstack which finds the
> > > information missing, or set to a particular values.
> >
> > Yes, I think that would be useful to add. In trying to implement the
> > associated changed in libxl I can see it would be valuable :-)
> 
> Right.  In particular, your specification needs to be compatible with
> the current implementation in libxl, as well as with a future
> implementation which actually uses these new features :-).
> 
> > > Likewise, they should document the expected behaviour of a guest
> which
> > > finds that the entries are missing or cannot be written for
> > > permissions reasons.
> >
> > In general from the guest PoV there's not a lot it can do if a path
> > is not writable, but I'll call that out.
> 
> Right.  But presumably you want to say that the guest may (try to)
> write them even if they don't already exist (that's right, isn't it?),
> but that the guest MUST NOT fail just because it can't write them.
> 
> If it can't write them it should simply carry on (maybe making a note
> in an obscure and non-alarming logfile somewhere).
> 

Yes, that's exactly the intention. I'll add suitable text.

  Paul

> thanks,
> Ian.