On Wed, Mar 14, 2018 at 17:05:30 +0100, 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 | 36 +++++ > src/libvirt_private.syms | 3 + > src/util/virstoragefile.c | 148 +++++++++++++++++++++ > src/util/virstoragefile.h | 15 +++ > .../disk-virtio-scsi-reservations-not-managed.xml | 41 ++++++ > .../disk-virtio-scsi-reservations.xml | 39 ++++++ > .../disk-virtio-scsi-reservations-not-managed.xml | 1 + > .../disk-virtio-scsi-reservations.xml | 1 + > tests/qemuxml2xmltest.c | 4 + > 12 files changed, 366 insertions(+), 31 deletions(-) > create mode 100644 tests/qemuxml2argvdata/disk-virtio-scsi-reservations-not-managed.xml > create mode 100644 tests/qemuxml2argvdata/disk-virtio-scsi-reservations.xml > create mode 120000 tests/qemuxml2xmloutdata/disk-virtio-scsi-reservations-not-managed.xml > create mode 120000 tests/qemuxml2xmloutdata/disk-virtio-scsi-reservations.xml > [skipping XML design since that was already agreed upon] > diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c > index 86fc27511..00b729fc2 100644 > --- a/src/conf/domain_conf.c > +++ b/src/conf/domain_conf.c > @@ -5201,6 +5201,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")); > + 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 > @@ -8586,6 +8593,29 @@ virDomainDiskSourcePrivateDataParse(xmlNodePtr node, > } > > > +static int > +virDomainDiskSourcePRParse(xmlNodePtr node, > + virStoragePRDefPtr *prsrc) > +{ > + xmlNodePtr child; > + virStoragePRDefPtr pr = NULL; > + > + for (child = node->children; child; child = child->next) { > + if (child->type == XML_ELEMENT_NODE && > + virXMLNodeNameEqual(child, "reservations")) { > + > + if (!(pr = virStoragePRDefParseNode(node->doc, child))) > + return -1; Use xpath instead of this ugly loop. I tried hard to remove it in my recent refactors of parsing the secret and encryption attributes. > + > + *prsrc = pr; > + return 0; > + } > + } > + > + return 0; > +} > + > + > static int > virDomainStorageSourceParse(xmlNodePtr node, > xmlXPathContextPtr ctxt, [...] > +static virStoragePRDefPtr > +virStoragePRDefParseXML(xmlXPathContextPtr ctxt) > +{ > + virStoragePRDefPtr prd, ret = NULL; > + char *enabled = NULL; > + char *managed = NULL; > + char *type = NULL; > + char *path = NULL; > + char *mode = NULL; > + > + if (VIR_ALLOC(prd) < 0) > + return NULL; > + > + if (!(enabled = virXPathString("string(./@enabled)", ctxt))) { > + virReportError(VIR_ERR_XML_ERROR, "%s", > + _("missing @enabled attribute for <reservations/>")); > + goto cleanup; > + } > + > + if ((prd->enabled = virTristateBoolTypeFromString(enabled)) <= 0) { > + virReportError(VIR_ERR_XML_ERROR, > + _("invalid value for 'enabled': %s"), NULLSTR(enabled)); 'enabled' can't be NULL here. > + goto cleanup; > + } > + > + if (prd->enabled == VIR_TRISTATE_BOOL_YES) { > + managed = virXPathString("string(./@managed)", ctxt); > + if ((prd->managed = virTristateBoolTypeFromString(managed)) <= 0) { > + virReportError(VIR_ERR_XML_ERROR, > + _("invalid value for 'managed': %s"), NULLSTR(managed)); > + goto cleanup; So this will report "invalid value for 'managed': <null>" rather than that it is missing. > + } > + > + if (prd->managed == VIR_TRISTATE_BOOL_NO) { > + type = virXPathString("string(./source[1]/@type)", ctxt); > + path = virXPathString("string(./source[1]/@path)", ctxt); > + mode = virXPathString("string(./source[1]/@mode)", ctxt); > + > + if (!type) { > + virReportError(VIR_ERR_XML_ERROR, "%s", > + _("missing connection type")); > + goto cleanup; > + } > + > + if (!path) { > + virReportError(VIR_ERR_XML_ERROR, "%s", > + _("missing path")); > + goto cleanup; > + } > + > + if (!mode) { > + virReportError(VIR_ERR_XML_ERROR, "%s", > + _("missing connection mode")); > + goto cleanup; All the above error messages don't really suggest which element is missing. > + } > + > + if (STRNEQ(type, "unix")) { > + virReportError(VIR_ERR_XML_ERROR, > + _("unsupported type: %s"), type); > + goto cleanup; > + } > + > + if (STRNEQ(mode, "client")) { > + virReportError(VIR_ERR_XML_ERROR, > + _("unsupported mode: %s"), mode); > + goto cleanup; > + } And these don't really say which value is wrong. > + > + prd->path = path; > + path = NULL; VIR_STEAL_PTR. Also 'mode' and type is not used after this. If you want to write it cleaner you could use a clever boolean xpath query to verify that both are present in the expected state. > + > + } > + } > + > + ret = prd; > + prd = NULL; > + > + cleanup: > + VIR_FREE(mode); > + VIR_FREE(path); > + VIR_FREE(type); > + VIR_FREE(managed); > + VIR_FREE(enabled); > + virStoragePRDefFree(prd); > + return ret; > +} > + > + > +virStoragePRDefPtr > +virStoragePRDefParseNode(xmlDocPtr xml, > + xmlNodePtr root) > +{ > + xmlXPathContextPtr ctxt = NULL; > + virStoragePRDefPtr prd = NULL; > + > + ctxt = xmlXPathNewContext(xml); > + if (ctxt == NULL) { > + virReportOOMError(); > + goto cleanup; > + } I've removed exactly this kind of code in recent refactors. Please pass the global context into the parser and don't add this function again. > + > + ctxt->node = root; > + prd = virStoragePRDefParseXML(ctxt); > + > + cleanup: > + xmlXPathFreeContext(ctxt); > + return prd; > +} > + > + > +void > +virStoragePRDefFormat(virBufferPtr buf, > + virStoragePRDefPtr prd) > +{ > + virBufferAsprintf(buf, "<reservations enabled='%s'", > + virTristateBoolTypeToString(prd->enabled)); > + if (prd->enabled == VIR_TRISTATE_BOOL_YES) { > + virBufferAsprintf(buf, " managed='%s'", > + virTristateBoolTypeToString(prd->managed)); > + if (prd->managed == VIR_TRISTATE_BOOL_NO) { > + virBufferAddLit(buf, ">\n"); > + virBufferAdjustIndent(buf, 2); > + virBufferAddLit(buf, "<source type='unix'"); > + virBufferEscapeString(buf, " path='%s'", prd->path); > + virBufferAddLit(buf, " mode='client'/>\n"); > + virBufferAdjustIndent(buf, -2); > + virBufferAddLit(buf, "</reservations>\n"); > + } else { > + virBufferAddLit(buf, "/>\n"); > + } > + } else { > + virBufferAddLit(buf, "/>\n"); > + } > +} > + > + > virSecurityDeviceLabelDefPtr > virStorageSourceGetSecurityLabelDef(virStorageSourcePtr src, > const char *model) > @@ -2270,6 +2417,7 @@ virStorageSourceClear(virStorageSourcePtr def) > virBitmapFree(def->features); > VIR_FREE(def->compat); > virStorageEncryptionFree(def->encryption); > + virStoragePRDefFree(def->pr); > virStorageSourceSeclabelsClear(def); > virStoragePermsFree(def->perms); > VIR_FREE(def->timestamps); > diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h > index 596746ccb..69fea34bc 100644 > --- a/src/util/virstoragefile.h > +++ b/src/util/virstoragefile.h > @@ -216,6 +216,14 @@ struct _virStorageAuthDef { > virSecretLookupTypeDef seclookupdef; > }; > > +typedef struct _virStoragePRDef virStoragePRDef; > +typedef virStoragePRDef *virStoragePRDefPtr; > +struct _virStoragePRDef { > + int enabled; /* enum virTristateBool */ > + int managed; /* enum virTristateBool */ > + char *path; > +}; > + > typedef struct _virStorageDriverData virStorageDriverData; > typedef virStorageDriverData *virStorageDriverDataPtr; > > @@ -243,6 +251,7 @@ struct _virStorageSource { > bool authInherited; > virStorageEncryptionPtr encryption; > bool encryptionInherited; > + virStoragePRDefPtr pr; > > virObjectPtr privateData; > > @@ -370,6 +379,12 @@ virStorageAuthDefPtr virStorageAuthDefParse(xmlNodePtr node, > xmlXPathContextPtr ctxt); > void virStorageAuthDefFormat(virBufferPtr buf, virStorageAuthDefPtr authdef); > > +void virStoragePRDefFree(virStoragePRDefPtr prd); > +virStoragePRDefPtr virStoragePRDefParseNode(xmlDocPtr xml, > + xmlNodePtr root); > +void virStoragePRDefFormat(virBufferPtr buf, > + virStoragePRDefPtr prd); > + > virSecurityDeviceLabelDefPtr > virStorageSourceGetSecurityLabelDef(virStorageSourcePtr src, > const char *model); > diff --git a/tests/qemuxml2argvdata/disk-virtio-scsi-reservations-not-managed.xml b/tests/qemuxml2argvdata/disk-virtio-scsi-reservations-not-managed.xml > new file mode 100644 > index 000000000..f4e4f1ee5 > --- /dev/null > +++ b/tests/qemuxml2argvdata/disk-virtio-scsi-reservations-not-managed.xml > @@ -0,0 +1,41 @@ > +<domain type='qemu'> > + <name>QEMUGuest1</name> > + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> > + <memory unit='KiB'>219136</memory> > + <currentMemory unit='KiB'>219136</currentMemory> > + <vcpu placement='static'>8</vcpu> > + <os> > + <type arch='i686' machine='pc'>hvm</type> > + <boot dev='hd'/> > + </os> > + <clock offset='utc'/> > + <on_poweroff>destroy</on_poweroff> > + <on_reboot>restart</on_reboot> > + <on_crash>destroy</on_crash> > + <devices> > + <emulator>/usr/bin/qemu-system-i686</emulator> > + <disk type='block' device='lun'> > + <driver name='qemu' type='raw'/> > + <source dev='/dev/HostVG/QEMUGuest1'> > + <reservations enabled='yes' managed='no'> > + <source type='unix' path='/path/to/qemu-pr-helper.sock' mode='client'/> > + </reservations> > + </source> > + <target dev='sdb' bus='scsi'/> > + <address type='drive' controller='0' bus='0' target='0' unit='0'/> > + </disk> > + <controller type='usb' index='0'> > + <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x2'/> > + </controller> > + <controller type='scsi' index='0' model='virtio-scsi'> > + <driver queues='8'/> > + <address type='pci' domain='0x0000' bus='0x00' slot='0x03' function='0x0'/> > + </controller> > + <controller type='pci' index='0' model='pci-root'/> > + <input type='mouse' bus='ps2'/> > + <input type='keyboard' bus='ps2'/> > + <memballoon model='virtio'> > + <address type='pci' domain='0x0000' bus='0x00' slot='0x04' function='0x0'/> > + </memballoon> > + </devices> > +</domain> > diff --git a/tests/qemuxml2argvdata/disk-virtio-scsi-reservations.xml b/tests/qemuxml2argvdata/disk-virtio-scsi-reservations.xml > new file mode 100644 > index 000000000..92225a5b0 > --- /dev/null > +++ b/tests/qemuxml2argvdata/disk-virtio-scsi-reservations.xml > @@ -0,0 +1,39 @@ > +<domain type='qemu'> > + <name>QEMUGuest1</name> > + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> > + <memory unit='KiB'>219136</memory> > + <currentMemory unit='KiB'>219136</currentMemory> > + <vcpu placement='static'>8</vcpu> > + <os> > + <type arch='i686' machine='pc'>hvm</type> > + <boot dev='hd'/> > + </os> > + <clock offset='utc'/> > + <on_poweroff>destroy</on_poweroff> > + <on_reboot>restart</on_reboot> > + <on_crash>destroy</on_crash> > + <devices> > + <emulator>/usr/bin/qemu-system-i686</emulator> > + <disk type='block' device='lun'> > + <driver name='qemu' type='raw'/> > + <source dev='/dev/HostVG/QEMUGuest1'> > + <reservations enabled='yes' managed='yes'/> > + </source> > + <target dev='sdb' bus='scsi'/> > + <address type='drive' controller='0' bus='0' target='0' unit='0'/> > + </disk> > + <controller type='usb' index='0'> > + <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x2'/> > + </controller> > + <controller type='scsi' index='0' model='virtio-scsi'> > + <driver queues='8'/> > + <address type='pci' domain='0x0000' bus='0x00' slot='0x03' function='0x0'/> > + </controller> > + <controller type='pci' index='0' model='pci-root'/> > + <input type='mouse' bus='ps2'/> > + <input type='keyboard' bus='ps2'/> > + <memballoon model='virtio'> > + <address type='pci' domain='0x0000' bus='0x00' slot='0x04' function='0x0'/> > + </memballoon> > + </devices> > +</domain> Looks like the two above XML files can be merged into one by adding two different disks.
Attachment:
signature.asc
Description: PGP signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
