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: > > <reservations enabled='yes' managed='no'> > <source type='unix' path='/path/to/qemu-pr-helper.sock' mode='client'/> > </reservations> > > If @managed is set to 'yes' then the <source/> 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 <mprivozn@xxxxxxxxxx> > --- > 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 120000 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 @@ > </disk> > <disk type='block' device='lun'> > <driver name='qemu' type='raw'/> > - <source dev='/dev/sda'/> > + <source dev='/dev/sda'> > + <reservations enabled='yes' managed='no'> > + <source type='unix' path='/path/to/qemu-pr-helper' mode='client'/> > + </reservations> > <target dev='sda' bus='scsi'/> > <address type='drive' controller='0' bus='0' target='3' unit='0'/> > </disk> > @@ -2926,6 +2929,26 @@ > <a href="formatstorageencryption.html">Storage Encryption</a> > page for more information. > </dd> > + <dt><code>reservations</code></dt> > + <dd><span class="since">Since libvirt 4.1.0</span>, the Now at least 4.3 > + <code>reservations</code> can be a sub-element of the > + <code>source</code> 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 > + <code>enabled</code> with accepted values <code>yes</code> and > + <code>no</code>. If the feature is enabled, then there's another <unrelatedRant>I wish we were consistent about using <code> around 'yes' and 'no' - some places use "yes" and/or "no".</unrelatedRant> > + mandatory attribute <code>managed</code> (accepted values are the > + same as for <code>enabled</code>) 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 <code>source</code>, which > + currently accepts only the following attributes: <code>type</code> > + with one value <code>unix</code>, <code>path</code> with path the > + socket, and finally <code>mode</code> which accepts either > + <code>server</code> or <code>client</code> 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. > + </dd> > </dl> > > <p> [...] > 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", > + _("<reservations/> allowed only for lun disks")); allowed or required ;-) s/disks/devices/ > + return -1; > + } > + > /* Reject disks with a bus type that is not compatible with the > * given address type. The function considers only buses that are > * handled in common code. For other bus types it's not possible [...] > static int > virDomainStorageSourceParse(xmlNodePtr node, > xmlXPathContextPtr ctxt, > @@ -8648,6 +8680,9 @@ virDomainStorageSourceParse(xmlNodePtr node, > !(src->encryption = virStorageEncryptionParseNode(tmp, ctxt))) > goto cleanup; > > + if (virDomainDiskSourcePRParse(node, ctxt, &src->pr) < 0) > + goto cleanup; > + > if (virSecurityDeviceLabelDefParseXML(&src->seclabels, &src->nseclabels, > ctxt, flags) < 0) > goto cleanup; > @@ -22927,6 +22962,9 @@ virDomainStorageSourceFormat(virBufferPtr attrBuf, > virStorageEncryptionFormat(childBuf, src->encryption) < 0) > return -1; > > + if (src->pr) > + virStoragePRDefFormat(childBuf, src->pr); Or just have virStoragePRDefFormat return if passed src->pr == NULL, IDC either way. > + > return 0; > } > [...] With at least the formatdomain and lun device error message adjustment, Reviewed-by: John Ferlan <jferlan@xxxxxxxxxx> John -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
