Re: [libvirt] [PATCH v4 01/14] virstoragefile: Introduce virStoragePRDef

2018-04-15 Thread John Ferlan


On 04/10/2018 10:58 AM, Michal Privoznik wrote:
> This is a definition that holds information on SCSI persistent
> reservation settings. The XML part looks like this:
> 
>   
> 
>   
> 
> If @managed is set to 'yes' then the  is not parsed.
> This design was agreed on here:
> 
> https://www.redhat.com/archives/libvir-list/2017-November/msg01005.html
> 
> Signed-off-by: Michal Privoznik 
> ---
>  docs/formatdomain.html.in  |  25 +++-
>  docs/schemas/domaincommon.rng  |  34 +-
>  docs/schemas/storagecommon.rng |  50 
>  src/conf/domain_conf.c |  38 ++
>  src/libvirt_private.syms   |   3 +
>  src/util/virstoragefile.c  | 131 
> +
>  src/util/virstoragefile.h  |  14 +++
>  .../disk-virtio-scsi-reservations.xml  |  49 
>  .../disk-virtio-scsi-reservations.xml  |   1 +
>  tests/qemuxml2xmltest.c|   2 +
>  10 files changed, 316 insertions(+), 31 deletions(-)
>  create mode 100644 tests/qemuxml2argvdata/disk-virtio-scsi-reservations.xml
>  create mode 12 tests/qemuxml2xmloutdata/disk-virtio-scsi-reservations.xml
> 
> diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
> index 5e99884dc5..c20e98b4ef 100644
> --- a/docs/formatdomain.html.in
> +++ b/docs/formatdomain.html.in
> @@ -2563,7 +2563,10 @@
>
>
>  
> -
> +
> +  
> + mode='client'/>
> +  
>  
>  
> > @@ -2926,6 +2929,26 @@ > Storage Encryption > page for more information. > > + reservations > + Since libvirt 4.1.0, the Now at least 4.3 > +reservations can be a sub-element of the > +source element for storage sources (QEMU driver > only). > +If present (and enabled) it enabled persistent reservations for s/enabled/enables > +SCSI based disks. The element has one mandatory attribute > +enabled with accepted values yes and > +no. If the feature is enabled, then there's another I wish we were consistent about using around 'yes' and 'no' - some places use "yes" and/or "no". > +mandatory attribute managed (accepted values are the > +same as for enabled) that enables or disables > libvirt > +spawning any helper process (should one be needed). However, if s/any/a/ ? s/(should one be needed)// IOW: What really decides if one is needed? > +libvirt is not enabled spawning helper process (i.e. hypervisor Starting from "However, if.. " This reads strange - why not just indicate "If enabled is yes, then libvirt will ...; otherwise, libvirt will Taken from patch 4: +/* Managed PR means there's one pr-manager object per domain + * and the pr-helper process is spawned and managed by + * libvirt. + * If PR is unmanaged there's one pr-manager object per disk + * (in general each disk can have its own pr-manager) and + * it's user's responsibility to start the pr-helper process. + */ When the PR is unmanaged, then the path to its socket... > +should just use one which is already running), path to its socket > +must be provided in child element source, which > +currently accepts only the following attributes: > type > +with one value unix, path with path the > +socket, and finally mode which accepts either > +server or client and specifies the role > +of hypervisor. mode only Parse's and Format's "client"... Perhaps the best thing to indicate here is that since libvirt is then a client of the user provided pr-helper process and will use a unix socket to connect to the process, the type must be unix and the mode must be client with the path being the path to the socket which libvirt will attempt to connect to. > + > > > [...] > diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c > index d23182f18a..5943d05e0e 100644 > --- a/src/conf/domain_conf.c > +++ b/src/conf/domain_conf.c > @@ -5210,6 +5210,13 @@ virDomainDiskDefValidate(const virDomainDiskDef *disk) > } > } > > +if (disk->src->pr && > +disk->device != VIR_DOMAIN_DISK_DEVICE_LUN) { > +virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", > + _(" allowed only for lun disks")); allowed or required ;-) s/disks/devices/ > +return -1; > +} > + > /* Reject dis

[libvirt] [PATCH v4 01/14] virstoragefile: Introduce virStoragePRDef

2018-04-10 Thread Michal Privoznik
This is a definition that holds information on SCSI persistent
reservation settings. The XML part looks like this:

  

  

If @managed is set to 'yes' then the  is not parsed.
This design was agreed on here:

https://www.redhat.com/archives/libvir-list/2017-November/msg01005.html

Signed-off-by: Michal Privoznik 
---
 docs/formatdomain.html.in  |  25 +++-
 docs/schemas/domaincommon.rng  |  34 +-
 docs/schemas/storagecommon.rng |  50 
 src/conf/domain_conf.c |  38 ++
 src/libvirt_private.syms   |   3 +
 src/util/virstoragefile.c  | 131 +
 src/util/virstoragefile.h  |  14 +++
 .../disk-virtio-scsi-reservations.xml  |  49 
 .../disk-virtio-scsi-reservations.xml  |   1 +
 tests/qemuxml2xmltest.c|   2 +
 10 files changed, 316 insertions(+), 31 deletions(-)
 create mode 100644 tests/qemuxml2argvdata/disk-virtio-scsi-reservations.xml
 create mode 12 tests/qemuxml2xmloutdata/disk-virtio-scsi-reservations.xml

diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
index 5e99884dc5..c20e98b4ef 100644
--- a/docs/formatdomain.html.in
+++ b/docs/formatdomain.html.in
@@ -2563,7 +2563,10 @@
   
   
 
-
+
+  
+
+  
 
 
@@ -2926,6 +2929,26 @@ Storage Encryption page for more information. + reservations + Since libvirt 4.1.0, the +reservations can be a sub-element of the +source element for storage sources (QEMU driver only). +If present (and enabled) it enabled persistent reservations for +SCSI based disks. The element has one mandatory attribute +enabled with accepted values yes and +no. If the feature is enabled, then there's another +mandatory attribute managed (accepted values are the +same as for enabled) that enables or disables libvirt +spawning any helper process (should one be needed). However, if +libvirt is not enabled spawning helper process (i.e. hypervisor +should just use one which is already running), path to its socket +must be provided in child element source, which +currently accepts only the following attributes: type +with one value unix, path with path the +socket, and finally mode which accepts either +server or client and specifies the role +of hypervisor. + diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index 4cab55f05d..93084887fb 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -1530,6 +1530,9 @@ + + + @@ -2434,18 +2437,6 @@ - - - - - - - - - - - -