[libvirt] [PATCH 00/10] conf: Make disk source parsing cleaner and reusable
Fixup and cleanup the disk source parsing code and make it usable in other parts of the code by exporting it. Peter Krempa (10): conf: Remove unnecessary condition from virDomainDiskSourceFormatInternal conf: Refactor seclabel formatting in virDomainDiskSourceFormatInternal conf: Remove virDomainDiskSourceDefFormatSeclabel conf: Refactor formatting of startupPolicy in virDomainDiskSourceFormatInternal conf: disk: Separate virStorageSource formatting conf: Validate disk source configuration also for the backing store conf: Separate seclabel validation from parsing conf: Parse and validate disk source seclabels together with the source conf: Extract parsing of storage source related data conf: Add and export wrapper for parsing storage source XML src/conf/domain_conf.c | 402 ++++++++++++++++++++++++++++------------------- src/conf/domain_conf.h | 13 ++ src/libvirt_private.syms | 2 + 3 files changed, 254 insertions(+), 163 deletions(-) -- 2.16.2
Now that the function is using virXMLFormatElement we don't need to conditionally format anything, since we'll format the element according to the presence of content. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/domain_conf.c | 126 ++++++++++++++++++++++++------------------------- 1 file changed, 62 insertions(+), 64 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 04a6ee77af..ddabc77a9b 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -22832,81 +22832,79 @@ virDomainDiskSourceFormatInternal(virBufferPtr buf, if (policy) startupPolicy = virDomainStartupPolicyTypeToString(policy); - if (src->path || src->nhosts > 0 || src->srcpool || startupPolicy) { - switch ((virStorageType)src->type) { - case VIR_STORAGE_TYPE_FILE: - virBufferEscapeString(&attrBuf, " file='%s'", src->path); - virBufferEscapeString(&attrBuf, " startupPolicy='%s'", startupPolicy); - - virDomainDiskSourceDefFormatSeclabel(&childBuf, src->nseclabels, - src->seclabels, flags, - skipSeclabels); - break; - - case VIR_STORAGE_TYPE_BLOCK: - virBufferEscapeString(&attrBuf, " dev='%s'", src->path); - virBufferEscapeString(&attrBuf, " startupPolicy='%s'", startupPolicy); - - virDomainDiskSourceDefFormatSeclabel(&childBuf, src->nseclabels, - src->seclabels, flags, - skipSeclabels); - break; + switch ((virStorageType)src->type) { + case VIR_STORAGE_TYPE_FILE: + virBufferEscapeString(&attrBuf, " file='%s'", src->path); + virBufferEscapeString(&attrBuf, " startupPolicy='%s'", startupPolicy); - case VIR_STORAGE_TYPE_DIR: - virBufferEscapeString(&attrBuf, " dir='%s'", src->path); - virBufferEscapeString(&attrBuf, " startupPolicy='%s'", startupPolicy); - break; + virDomainDiskSourceDefFormatSeclabel(&childBuf, src->nseclabels, + src->seclabels, flags, + skipSeclabels); + break; - case VIR_STORAGE_TYPE_NETWORK: - if (virDomainDiskSourceFormatNetwork(&attrBuf, &childBuf, - src, flags) < 0) - goto error; - break; + case VIR_STORAGE_TYPE_BLOCK: + virBufferEscapeString(&attrBuf, " dev='%s'", src->path); + virBufferEscapeString(&attrBuf, " startupPolicy='%s'", startupPolicy); - case VIR_STORAGE_TYPE_VOLUME: - if (src->srcpool) { - virBufferEscapeString(&attrBuf, " pool='%s'", src->srcpool->pool); - virBufferEscapeString(&attrBuf, " volume='%s'", - src->srcpool->volume); - if (src->srcpool->mode) - virBufferAsprintf(&attrBuf, " mode='%s'", - virStorageSourcePoolModeTypeToString(src->srcpool->mode)); - } - virBufferEscapeString(&attrBuf, " startupPolicy='%s'", startupPolicy); + virDomainDiskSourceDefFormatSeclabel(&childBuf, src->nseclabels, + src->seclabels, flags, + skipSeclabels); + break; - virDomainDiskSourceDefFormatSeclabel(&childBuf, src->nseclabels, - src->seclabels, flags, - skipSeclabels); - break; + case VIR_STORAGE_TYPE_DIR: + virBufferEscapeString(&attrBuf, " dir='%s'", src->path); + virBufferEscapeString(&attrBuf, " startupPolicy='%s'", startupPolicy); + break; - case VIR_STORAGE_TYPE_NONE: - case VIR_STORAGE_TYPE_LAST: - virReportError(VIR_ERR_INTERNAL_ERROR, - _("unexpected disk type %d"), src->type); + case VIR_STORAGE_TYPE_NETWORK: + if (virDomainDiskSourceFormatNetwork(&attrBuf, &childBuf, + src, flags) < 0) goto error; + break; + + case VIR_STORAGE_TYPE_VOLUME: + if (src->srcpool) { + virBufferEscapeString(&attrBuf, " pool='%s'", src->srcpool->pool); + virBufferEscapeString(&attrBuf, " volume='%s'", + src->srcpool->volume); + if (src->srcpool->mode) + virBufferAsprintf(&attrBuf, " mode='%s'", + virStorageSourcePoolModeTypeToString(src->srcpool->mode)); } + virBufferEscapeString(&attrBuf, " startupPolicy='%s'", startupPolicy); - /* Storage Source formatting will not carry through the blunder - * that disk source formatting had at one time to format the - * <auth> for a volume source type. The <auth> information is - * kept in the storage pool and would be overwritten anyway. - * So avoid formatting it for volumes. */ - if (src->auth && src->authInherited && - src->type != VIR_STORAGE_TYPE_VOLUME) - virStorageAuthDefFormat(&childBuf, src->auth); + virDomainDiskSourceDefFormatSeclabel(&childBuf, src->nseclabels, + src->seclabels, flags, + skipSeclabels); + break; - /* If we found encryption as a child of <source>, then format it - * as we found it. */ - if (src->encryption && src->encryptionInherited && - virStorageEncryptionFormat(&childBuf, src->encryption) < 0) - goto error; + case VIR_STORAGE_TYPE_NONE: + case VIR_STORAGE_TYPE_LAST: + virReportError(VIR_ERR_INTERNAL_ERROR, + _("unexpected disk type %d"), src->type); + goto error; + } - if (virDomainDiskSourceFormatPrivateData(&childBuf, src, flags, xmlopt) < 0) - goto error; + /* Storage Source formatting will not carry through the blunder + * that disk source formatting had at one time to format the + * <auth> for a volume source type. The <auth> information is + * kept in the storage pool and would be overwritten anyway. + * So avoid formatting it for volumes. */ + if (src->auth && src->authInherited && + src->type != VIR_STORAGE_TYPE_VOLUME) + virStorageAuthDefFormat(&childBuf, src->auth); - if (virXMLFormatElement(buf, "source", &attrBuf, &childBuf) < 0) - goto error; - } + /* If we found encryption as a child of <source>, then format it + * as we found it. */ + if (src->encryption && src->encryptionInherited && + virStorageEncryptionFormat(&childBuf, src->encryption) < 0) + goto error; + + if (virDomainDiskSourceFormatPrivateData(&childBuf, src, flags, xmlopt) < 0) + goto error; + + if (virXMLFormatElement(buf, "source", &attrBuf, &childBuf) < 0) + goto error; return 0; -- 2.16.2
Call the formatter function only once. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/domain_conf.c | 17 ++++++----------- 1 file changed, 6 insertions(+), 11 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index ddabc77a9b..ebe1172fd2 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -22836,19 +22836,11 @@ virDomainDiskSourceFormatInternal(virBufferPtr buf, case VIR_STORAGE_TYPE_FILE: virBufferEscapeString(&attrBuf, " file='%s'", src->path); virBufferEscapeString(&attrBuf, " startupPolicy='%s'", startupPolicy); - - virDomainDiskSourceDefFormatSeclabel(&childBuf, src->nseclabels, - src->seclabels, flags, - skipSeclabels); break; case VIR_STORAGE_TYPE_BLOCK: virBufferEscapeString(&attrBuf, " dev='%s'", src->path); virBufferEscapeString(&attrBuf, " startupPolicy='%s'", startupPolicy); - - virDomainDiskSourceDefFormatSeclabel(&childBuf, src->nseclabels, - src->seclabels, flags, - skipSeclabels); break; case VIR_STORAGE_TYPE_DIR: @@ -22873,9 +22865,6 @@ virDomainDiskSourceFormatInternal(virBufferPtr buf, } virBufferEscapeString(&attrBuf, " startupPolicy='%s'", startupPolicy); - virDomainDiskSourceDefFormatSeclabel(&childBuf, src->nseclabels, - src->seclabels, flags, - skipSeclabels); break; case VIR_STORAGE_TYPE_NONE: @@ -22885,6 +22874,12 @@ virDomainDiskSourceFormatInternal(virBufferPtr buf, goto error; } + if (src->type != VIR_STORAGE_TYPE_NETWORK) { + virDomainDiskSourceDefFormatSeclabel(&childBuf, src->nseclabels, + src->seclabels, flags, + skipSeclabels); + } + /* Storage Source formatting will not carry through the blunder * that disk source formatting had at one time to format the * <auth> for a volume source type. The <auth> information is -- 2.16.2
The wrapper functionality can be moved to the only user virDomainDiskSourceFormatInternal. Also removes comment which does not reflect the truth any more. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/domain_conf.c | 31 +++++++------------------------ 1 file changed, 7 insertions(+), 24 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index ebe1172fd2..4aa66fe09c 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -22704,33 +22704,16 @@ virDomainDiskBlockIoDefFormat(virBufferPtr buf, } -/* virDomainSourceDefFormatSeclabel: - * - * This function automatically closes the <source> element and formats any - * possible seclabels. - */ -static void -virDomainDiskSourceDefFormatSeclabel(virBufferPtr buf, - size_t nseclabels, - virSecurityDeviceLabelDefPtr *seclabels, - unsigned int flags, - bool skipSeclables) -{ - size_t n; - - if (nseclabels && !skipSeclables) { - for (n = 0; n < nseclabels; n++) - virSecurityDeviceLabelDefFormat(buf, seclabels[n], flags); - } -} - static void virDomainSourceDefFormatSeclabel(virBufferPtr buf, size_t nseclabels, virSecurityDeviceLabelDefPtr *seclabels, unsigned int flags) { - virDomainDiskSourceDefFormatSeclabel(buf, nseclabels, seclabels, flags, false); + size_t n; + + for (n = 0; n < nseclabels; n++) + virSecurityDeviceLabelDefFormat(buf, seclabels[n], flags); } @@ -22875,9 +22858,9 @@ virDomainDiskSourceFormatInternal(virBufferPtr buf, } if (src->type != VIR_STORAGE_TYPE_NETWORK) { - virDomainDiskSourceDefFormatSeclabel(&childBuf, src->nseclabels, - src->seclabels, flags, - skipSeclabels); + if (!skipSeclabels) + virDomainSourceDefFormatSeclabel(&childBuf, src->nseclabels, + src->seclabels, flags); } /* Storage Source formatting will not carry through the blunder -- 2.16.2
Move it to a single location which also allows to get rid of the temporrary variable. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/domain_conf.c | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 4aa66fe09c..b77cc8ed9f 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -22806,29 +22806,22 @@ virDomainDiskSourceFormatInternal(virBufferPtr buf, bool skipSeclabels, virDomainXMLOptionPtr xmlopt) { - const char *startupPolicy = NULL; virBuffer attrBuf = VIR_BUFFER_INITIALIZER; virBuffer childBuf = VIR_BUFFER_INITIALIZER; virBufferSetChildIndent(&childBuf, buf); - if (policy) - startupPolicy = virDomainStartupPolicyTypeToString(policy); - switch ((virStorageType)src->type) { case VIR_STORAGE_TYPE_FILE: virBufferEscapeString(&attrBuf, " file='%s'", src->path); - virBufferEscapeString(&attrBuf, " startupPolicy='%s'", startupPolicy); break; case VIR_STORAGE_TYPE_BLOCK: virBufferEscapeString(&attrBuf, " dev='%s'", src->path); - virBufferEscapeString(&attrBuf, " startupPolicy='%s'", startupPolicy); break; case VIR_STORAGE_TYPE_DIR: virBufferEscapeString(&attrBuf, " dir='%s'", src->path); - virBufferEscapeString(&attrBuf, " startupPolicy='%s'", startupPolicy); break; case VIR_STORAGE_TYPE_NETWORK: @@ -22846,7 +22839,6 @@ virDomainDiskSourceFormatInternal(virBufferPtr buf, virBufferAsprintf(&attrBuf, " mode='%s'", virStorageSourcePoolModeTypeToString(src->srcpool->mode)); } - virBufferEscapeString(&attrBuf, " startupPolicy='%s'", startupPolicy); break; @@ -22858,6 +22850,10 @@ virDomainDiskSourceFormatInternal(virBufferPtr buf, } if (src->type != VIR_STORAGE_TYPE_NETWORK) { + if (policy) + virBufferEscapeString(&attrBuf, " startupPolicy='%s'", + virDomainStartupPolicyTypeToString(policy)); + if (!skipSeclabels) virDomainSourceDefFormatSeclabel(&childBuf, src->nseclabels, src->seclabels, flags); -- 2.16.2
Move out formatting of 'startuPolicy' which is a property of the disk out of the <source> element. Extracting the code formating the content and attributes will also allow reuse in other parts of the code. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/domain_conf.c | 90 ++++++++++++++++++++++++++++-------------------- src/conf/domain_conf.h | 7 ++++ src/libvirt_private.syms | 1 + 3 files changed, 60 insertions(+), 38 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index b77cc8ed9f..1c79d2b49b 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -22798,45 +22798,39 @@ virDomainDiskSourceFormatPrivateData(virBufferPtr buf, } -static int -virDomainDiskSourceFormatInternal(virBufferPtr buf, - virStorageSourcePtr src, - int policy, - unsigned int flags, - bool skipSeclabels, - virDomainXMLOptionPtr xmlopt) +int +virDomainStorageSourceFormat(virBufferPtr attrBuf, + virBufferPtr childBuf, + virStorageSourcePtr src, + unsigned int flags, + bool skipSeclabels) { - virBuffer attrBuf = VIR_BUFFER_INITIALIZER; - virBuffer childBuf = VIR_BUFFER_INITIALIZER; - - virBufferSetChildIndent(&childBuf, buf); - switch ((virStorageType)src->type) { case VIR_STORAGE_TYPE_FILE: - virBufferEscapeString(&attrBuf, " file='%s'", src->path); + virBufferEscapeString(attrBuf, " file='%s'", src->path); break; case VIR_STORAGE_TYPE_BLOCK: - virBufferEscapeString(&attrBuf, " dev='%s'", src->path); + virBufferEscapeString(attrBuf, " dev='%s'", src->path); break; case VIR_STORAGE_TYPE_DIR: - virBufferEscapeString(&attrBuf, " dir='%s'", src->path); + virBufferEscapeString(attrBuf, " dir='%s'", src->path); break; case VIR_STORAGE_TYPE_NETWORK: - if (virDomainDiskSourceFormatNetwork(&attrBuf, &childBuf, + if (virDomainDiskSourceFormatNetwork(attrBuf, childBuf, src, flags) < 0) - goto error; + return -1; break; case VIR_STORAGE_TYPE_VOLUME: if (src->srcpool) { - virBufferEscapeString(&attrBuf, " pool='%s'", src->srcpool->pool); - virBufferEscapeString(&attrBuf, " volume='%s'", + virBufferEscapeString(attrBuf, " pool='%s'", src->srcpool->pool); + virBufferEscapeString(attrBuf, " volume='%s'", src->srcpool->volume); if (src->srcpool->mode) - virBufferAsprintf(&attrBuf, " mode='%s'", + virBufferAsprintf(attrBuf, " mode='%s'", virStorageSourcePoolModeTypeToString(src->srcpool->mode)); } @@ -22846,18 +22840,12 @@ virDomainDiskSourceFormatInternal(virBufferPtr buf, case VIR_STORAGE_TYPE_LAST: virReportError(VIR_ERR_INTERNAL_ERROR, _("unexpected disk type %d"), src->type); - goto error; + return -1; } - if (src->type != VIR_STORAGE_TYPE_NETWORK) { - if (policy) - virBufferEscapeString(&attrBuf, " startupPolicy='%s'", - virDomainStartupPolicyTypeToString(policy)); - - if (!skipSeclabels) - virDomainSourceDefFormatSeclabel(&childBuf, src->nseclabels, - src->seclabels, flags); - } + if (!skipSeclabels && src->type != VIR_STORAGE_TYPE_NETWORK) + virDomainSourceDefFormatSeclabel(childBuf, src->nseclabels, + src->seclabels, flags); /* Storage Source formatting will not carry through the blunder * that disk source formatting had at one time to format the @@ -22866,26 +22854,52 @@ virDomainDiskSourceFormatInternal(virBufferPtr buf, * So avoid formatting it for volumes. */ if (src->auth && src->authInherited && src->type != VIR_STORAGE_TYPE_VOLUME) - virStorageAuthDefFormat(&childBuf, src->auth); + virStorageAuthDefFormat(childBuf, src->auth); /* If we found encryption as a child of <source>, then format it * as we found it. */ if (src->encryption && src->encryptionInherited && - virStorageEncryptionFormat(&childBuf, src->encryption) < 0) - goto error; + virStorageEncryptionFormat(childBuf, src->encryption) < 0) + return -1; + + return 0; +} + + +static int +virDomainDiskSourceFormatInternal(virBufferPtr buf, + virStorageSourcePtr src, + int policy, + unsigned int flags, + bool skipSeclabels, + virDomainXMLOptionPtr xmlopt) +{ + virBuffer attrBuf = VIR_BUFFER_INITIALIZER; + virBuffer childBuf = VIR_BUFFER_INITIALIZER; + int ret = -1; + + virBufferSetChildIndent(&childBuf, buf); + + if (virDomainStorageSourceFormat(&attrBuf, &childBuf, src, flags, + skipSeclabels) < 0) + goto cleanup; + + if (policy && src->type != VIR_STORAGE_TYPE_NETWORK) + virBufferEscapeString(&attrBuf, " startupPolicy='%s'", + virDomainStartupPolicyTypeToString(policy)); if (virDomainDiskSourceFormatPrivateData(&childBuf, src, flags, xmlopt) < 0) - goto error; + goto cleanup; if (virXMLFormatElement(buf, "source", &attrBuf, &childBuf) < 0) - goto error; + goto cleanup; - return 0; + ret = 0; - error: + cleanup: virBufferFreeAndReset(&attrBuf); virBufferFreeAndReset(&childBuf); - return -1; + return ret; } diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 337ce79425..61379e50fe 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -3427,6 +3427,13 @@ int virDomainDiskDefCheckDuplicateInfo(const virDomainDiskDef *a, const virDomainDiskDef *b) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); +int virDomainStorageSourceFormat(virBufferPtr attrBuf, + virBufferPtr childBuf, + virStorageSourcePtr src, + unsigned int flags, + bool skipSeclabels) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3); + int virDomainDefGetVcpuPinInfoHelper(virDomainDefPtr def, int maplen, int ncpumaps, diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 3766e20d3b..c67bce7389 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -542,6 +542,7 @@ virDomainStateReasonFromString; virDomainStateReasonToString; virDomainStateTypeFromString; virDomainStateTypeToString; +virDomainStorageSourceFormat; virDomainTaintTypeFromString; virDomainTaintTypeToString; virDomainTimerModeTypeFromString; -- 2.16.2
Since we already parse the <backingStore> of a disk source, we should also validate the configuration for the whole backing chain and not only for the top level image. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/domain_conf.c | 24 ++++++++++++++---------- 1 file changed, 14 insertions(+), 10 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 1c79d2b49b..8cd41edb5e 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -8974,6 +8974,8 @@ virDomainDiskSourceDefParseAuthValidate(const virStorageSource *src) static int virDomainDiskDefParseValidate(const virDomainDiskDef *def) { + virStorageSourcePtr next; + if (def->bus != VIR_DOMAIN_DISK_BUS_VIRTIO) { if (def->event_idx != VIR_TRISTATE_SWITCH_ABSENT) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", @@ -9044,19 +9046,21 @@ virDomainDiskDefParseValidate(const virDomainDiskDef *def) } } - if (virDomainDiskSourceDefParseAuthValidate(def->src) < 0) - return -1; + for (next = def->src; next; next = next->backingStore) { + if (virDomainDiskSourceDefParseAuthValidate(next) < 0) + return -1; - if (def->src->encryption) { - virStorageEncryptionPtr encryption = def->src->encryption; + if (next->encryption) { + virStorageEncryptionPtr encryption = next->encryption; - if (encryption->format == VIR_STORAGE_ENCRYPTION_FORMAT_LUKS && - encryption->encinfo.cipher_name) { + if (encryption->format == VIR_STORAGE_ENCRYPTION_FORMAT_LUKS && + encryption->encinfo.cipher_name) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("supplying <cipher> for domain disk definition " - "is unnecessary")); - return -1; + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("supplying <cipher> for domain disk definition " + "is unnecessary")); + return -1; + } } } -- 2.16.2
Rather than checking that the security label is legal when parsing it move the code into a separate function. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/domain_conf.c | 68 ++++++++++++++++++++++++++++++++------------------ 1 file changed, 44 insertions(+), 24 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 8cd41edb5e..6c2a2f3a75 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -8214,8 +8214,7 @@ virSecurityLabelDefsParseXML(virDomainDefPtr def, static int virSecurityDeviceLabelDefParseXML(virSecurityDeviceLabelDefPtr **seclabels_rtn, size_t *nseclabels_rtn, - virSecurityLabelDefPtr *vmSeclabels, - int nvmSeclabels, xmlXPathContextPtr ctxt, + xmlXPathContextPtr ctxt, unsigned int flags) { virSecurityDeviceLabelDefPtr *seclabels = NULL; @@ -8223,7 +8222,6 @@ virSecurityDeviceLabelDefParseXML(virSecurityDeviceLabelDefPtr **seclabels_rtn, int n; size_t i, j; xmlNodePtr *list = NULL; - virSecurityLabelDefPtr vmDef = NULL; char *model, *relabel, *label, *labelskip; if ((n = virXPathNodeSet("./seclabel", ctxt, &list)) < 0) @@ -8243,14 +8241,6 @@ virSecurityDeviceLabelDefParseXML(virSecurityDeviceLabelDefPtr **seclabels_rtn, /* get model associated to this override */ model = virXMLPropString(list[i], "model"); if (model) { - /* find the security label that it's being overridden */ - for (j = 0; j < nvmSeclabels; j++) { - if (STREQ(vmSeclabels[j]->model, model)) { - vmDef = vmSeclabels[j]; - break; - } - } - /* check for duplicate seclabels */ for (j = 0; j < i; j++) { if (STREQ_NULLABLE(model, seclabels[j]->model)) { @@ -8262,14 +8252,6 @@ virSecurityDeviceLabelDefParseXML(virSecurityDeviceLabelDefPtr **seclabels_rtn, seclabels[i]->model = model; } - /* Can't use overrides if top-level doesn't allow relabeling. */ - if (vmDef && !vmDef->relabel) { - virReportError(VIR_ERR_XML_ERROR, "%s", - _("label overrides require relabeling to be " - "enabled at the domain level")); - goto error; - } - relabel = virXMLPropString(list[i], "relabel"); if (relabel != NULL) { if (STREQ(relabel, "yes")) { @@ -8324,6 +8306,37 @@ virSecurityDeviceLabelDefParseXML(virSecurityDeviceLabelDefPtr **seclabels_rtn, } +static int +virSecurityDeviceLabelDefValidateXML(virSecurityDeviceLabelDefPtr *seclabels, + size_t nseclabels, + virSecurityLabelDefPtr *vmSeclabels, + size_t nvmSeclabels) +{ + virSecurityDeviceLabelDefPtr seclabel; + size_t i; + size_t j; + + for (i = 0; i < nseclabels; i++) { + seclabel = seclabels[i]; + + /* find the security label that it's being overridden */ + for (j = 0; j < nvmSeclabels; j++) { + if (STRNEQ_NULLABLE(vmSeclabels[j]->model, seclabel->model)) + continue; + + if (!vmSeclabels[j]->relabel) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("label overrides require relabeling to be " + "enabled at the domain level")); + return -1; + } + } + } + + return 0; +} + + /* Parse the XML definition for a lease */ static virDomainLeaseDefPtr @@ -9453,11 +9466,16 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt, ctxt->node = sourceNode; if (virSecurityDeviceLabelDefParseXML(&def->src->seclabels, &def->src->nseclabels, - vmSeclabels, - nvmSeclabels, ctxt, flags) < 0) goto error; + + if (virSecurityDeviceLabelDefValidateXML(def->src->seclabels, + def->src->nseclabels, + vmSeclabels, + nvmSeclabels) < 0) + goto error; + ctxt->node = saved_node; } @@ -12133,10 +12151,12 @@ virDomainChrSourceDefParseXML(virDomainChrSourceDefPtr def, ctxt->node = cur; if (virSecurityDeviceLabelDefParseXML(&def->seclabels, &def->nseclabels, - vmSeclabels, - nvmSeclabels, ctxt, - flags) < 0) { + flags) < 0 || + virSecurityDeviceLabelDefValidateXML(def->seclabels, + def->nseclabels, + vmSeclabels, + nvmSeclabels) < 0) { ctxt->node = saved_node; goto error; } -- 2.16.2
Since seclabels are formatted along with the source element and will also make sense to be passed for the backing chain we should parse them in the place where we parse the disk source. Same applies for validation. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/domain_conf.c | 39 +++++++++++++++------------------------ 1 file changed, 15 insertions(+), 24 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 6c2a2f3a75..d1ff80feb7 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -8631,6 +8631,10 @@ virDomainDiskSourceParse(xmlNodePtr node, !(src->encryption = virStorageEncryptionParseNode(tmp, ctxt))) goto cleanup; + if (virSecurityDeviceLabelDefParseXML(&src->seclabels, &src->nseclabels, + ctxt, flags) < 0) + goto cleanup; + if (virDomainDiskSourcePrivateDataParse(ctxt, src, flags, xmlopt) < 0) goto cleanup; @@ -8985,7 +8989,10 @@ virDomainDiskSourceDefParseAuthValidate(const virStorageSource *src) static int -virDomainDiskDefParseValidate(const virDomainDiskDef *def) +virDomainDiskDefParseValidate(const virDomainDiskDef *def, + virSecurityLabelDefPtr *vmSeclabels, + size_t nvmSeclabels) + { virStorageSourcePtr next; @@ -9075,6 +9082,12 @@ virDomainDiskDefParseValidate(const virDomainDiskDef *def) return -1; } } + + if (virSecurityDeviceLabelDefValidateXML(next->seclabels, + next->nseclabels, + vmSeclabels, + nvmSeclabels) < 0) + return -1; } return 0; @@ -9222,7 +9235,6 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt, unsigned int flags) { virDomainDiskDefPtr def; - xmlNodePtr sourceNode = NULL; xmlNodePtr cur; xmlNodePtr save_ctxt = ctxt->node; char *tmp = NULL; @@ -9281,8 +9293,6 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt, continue; if (!source && virXMLNodeNameEqual(cur, "source")) { - sourceNode = cur; - if (virDomainDiskSourceParse(cur, ctxt, def->src, flags, xmlopt) < 0) goto error; @@ -9460,25 +9470,6 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt, goto error; } - /* If source is present, check for an optional seclabel override. */ - if (sourceNode) { - xmlNodePtr saved_node = ctxt->node; - ctxt->node = sourceNode; - if (virSecurityDeviceLabelDefParseXML(&def->src->seclabels, - &def->src->nseclabels, - ctxt, - flags) < 0) - goto error; - - if (virSecurityDeviceLabelDefValidateXML(def->src->seclabels, - def->src->nseclabels, - vmSeclabels, - nvmSeclabels) < 0) - goto error; - - ctxt->node = saved_node; - } - if (!target && !(flags & VIR_DOMAIN_DEF_PARSE_DISK_SOURCE)) { if (def->src->srcpool) { if (virAsprintf(&tmp, "pool = '%s', volume = '%s'", @@ -9644,7 +9635,7 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt, goto error; } - if (virDomainDiskDefParseValidate(def) < 0) + if (virDomainDiskDefParseValidate(def, vmSeclabels, nvmSeclabels) < 0) goto error; cleanup: -- 2.16.2
Split out the parser and separate it from the private data part so that it can be later reused in other parts of the code. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/domain_conf.c | 43 +++++++++++++++++++++++++++++-------------- 1 file changed, 29 insertions(+), 14 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index d1ff80feb7..86fc275116 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -8554,24 +8554,26 @@ virDomainDiskSourceNetworkParse(xmlNodePtr node, static int -virDomainDiskSourcePrivateDataParse(xmlXPathContextPtr ctxt, +virDomainDiskSourcePrivateDataParse(xmlNodePtr node, + xmlXPathContextPtr ctxt, virStorageSourcePtr src, unsigned int flags, virDomainXMLOptionPtr xmlopt) { xmlNodePtr saveNode = ctxt->node; - xmlNodePtr node; int ret = -1; if (!(flags & VIR_DOMAIN_DEF_PARSE_STATUS) || !xmlopt || !xmlopt->privateData.storageParse) return 0; - if (!(node = virXPathNode("./privateData", ctxt))) - return 0; - ctxt->node = node; + if (!(ctxt->node = virXPathNode("./privateData", ctxt))) { + ret = 0; + goto cleanup; + } + if (xmlopt->privateData.storageParse(ctxt, src) < 0) goto cleanup; @@ -8584,12 +8586,11 @@ virDomainDiskSourcePrivateDataParse(xmlXPathContextPtr ctxt, } -int -virDomainDiskSourceParse(xmlNodePtr node, - xmlXPathContextPtr ctxt, - virStorageSourcePtr src, - unsigned int flags, - virDomainXMLOptionPtr xmlopt) +static int +virDomainStorageSourceParse(xmlNodePtr node, + xmlXPathContextPtr ctxt, + virStorageSourcePtr src, + unsigned int flags) { int ret = -1; xmlNodePtr saveNode = ctxt->node; @@ -8635,9 +8636,6 @@ virDomainDiskSourceParse(xmlNodePtr node, ctxt, flags) < 0) goto cleanup; - if (virDomainDiskSourcePrivateDataParse(ctxt, src, flags, xmlopt) < 0) - goto cleanup; - /* People sometimes pass a bogus '' source path when they mean to omit the * source element completely (e.g. CDROM without media). This is just a * little compatibility check to help those broken apps */ @@ -8652,6 +8650,23 @@ virDomainDiskSourceParse(xmlNodePtr node, } +int +virDomainDiskSourceParse(xmlNodePtr node, + xmlXPathContextPtr ctxt, + virStorageSourcePtr src, + unsigned int flags, + virDomainXMLOptionPtr xmlopt) +{ + if (virDomainStorageSourceParse(node, ctxt, src, flags) < 0) + return -1; + + if (virDomainDiskSourcePrivateDataParse(node, ctxt, src, flags, xmlopt) < 0) + return -1; + + return 0; +} + + static int virDomainDiskBackingStoreParse(xmlXPathContextPtr ctxt, virStorageSourcePtr src, -- 2.16.2
Add a helper that parses a storage source XML node into a new virStorageSource object. Since there are multiple approaches to store the 'type' and 'format' attributes, they need to be parsed manually prior to calling the function. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/domain_conf.c | 60 ++++++++++++++++++++++++++++++++++++++++++++++++ src/conf/domain_conf.h | 6 +++++ src/libvirt_private.syms | 1 + 3 files changed, 67 insertions(+) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 86fc275116..0b25c6316f 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -8650,6 +8650,66 @@ virDomainStorageSourceParse(xmlNodePtr node, } +/** + * virDomainStorageSourceParseNew: + * @node: XML node object to parse + * @ctxt: XML XPath context + * @flags: virDomainDefParseFlags + * + * Parses the XML @node and returns a virStorageSource object with the parsed + * data. Note that 'format' and 'type' attributes need to be members of the same + * object and need to be provided. + */ +virStorageSourcePtr +virDomainStorageSourceParseNew(xmlNodePtr node, + xmlXPathContextPtr ctxt, + unsigned int flags) +{ + virStorageSourcePtr src = NULL; + virStorageSourcePtr ret = NULL; + char *format = NULL; + char *type = NULL; + + if (VIR_ALLOC(src) < 0) + return NULL; + + if (!(type = virXMLPropString(node, "type"))) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("missing storage source type")); + goto cleanup; + } + + if (!(format = virXMLPropString(node, "format"))) { + virReportError(VIR_ERR_XML_ERROR, "%s", + ("missing storage source format")); + goto cleanup; + } + + if ((src->type = virStorageTypeFromString(type)) <= 0) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("unknown storage source type '%s'"), type); + goto cleanup; + } + + if ((src->format = virStorageFileFormatTypeFromString(format)) <= 0) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("unknown storage source format '%s'"), format); + goto cleanup; + } + + if (virDomainStorageSourceParse(node, ctxt, src, flags) < 0) + goto cleanup; + + VIR_STEAL_PTR(ret, src); + + cleanup: + virStorageSourceFree(src); + VIR_FREE(format); + VIR_FREE(type); + return ret; +} + + int virDomainDiskSourceParse(xmlNodePtr node, xmlXPathContextPtr ctxt, diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 61379e50fe..c82a23d220 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -3434,6 +3434,12 @@ int virDomainStorageSourceFormat(virBufferPtr attrBuf, bool skipSeclabels) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3); +virStorageSourcePtr +virDomainStorageSourceParseNew(xmlNodePtr node, + xmlXPathContextPtr ctxt, + unsigned int flags) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); + int virDomainDefGetVcpuPinInfoHelper(virDomainDefPtr def, int maplen, int ncpumaps, diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index c67bce7389..4dfcbb4230 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -543,6 +543,7 @@ virDomainStateReasonToString; virDomainStateTypeFromString; virDomainStateTypeToString; virDomainStorageSourceFormat; +virDomainStorageSourceParseNew; virDomainTaintTypeFromString; virDomainTaintTypeToString; virDomainTimerModeTypeFromString; -- 2.16.2
On 03/13/2018 03:37 PM, Peter Krempa wrote:
Add a helper that parses a storage source XML node into a new virStorageSource object. Since there are multiple approaches to store the 'type' and 'format' attributes, they need to be parsed manually prior to calling the function.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/domain_conf.c | 60 ++++++++++++++++++++++++++++++++++++++++++++++++ src/conf/domain_conf.h | 6 +++++ src/libvirt_private.syms | 1 + 3 files changed, 67 insertions(+)
Perhaps you forgot to include next patch that uses virDomainStorageSourceParseNew()? Because this patch does nothing more than introducing a function that is never used. Michal
On Wed, Mar 14, 2018 at 11:00:46 +0100, Michal Privoznik wrote:
On 03/13/2018 03:37 PM, Peter Krempa wrote:
Add a helper that parses a storage source XML node into a new virStorageSource object. Since there are multiple approaches to store the 'type' and 'format' attributes, they need to be parsed manually prior to calling the function.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/domain_conf.c | 60 ++++++++++++++++++++++++++++++++++++++++++++++++ src/conf/domain_conf.h | 6 +++++ src/libvirt_private.syms | 1 + 3 files changed, 67 insertions(+)
Perhaps you forgot to include next patch that uses virDomainStorageSourceParseNew()? Because this patch does nothing more than introducing a function that is never used.
Yes. The patch that uses this function was not posted yet. I'm sending these sub-series out so that I don't have to send 50+ patches at once.
On 03/14/2018 11:57 AM, Peter Krempa wrote:
On Wed, Mar 14, 2018 at 11:00:46 +0100, Michal Privoznik wrote:
On 03/13/2018 03:37 PM, Peter Krempa wrote:
Add a helper that parses a storage source XML node into a new virStorageSource object. Since there are multiple approaches to store the 'type' and 'format' attributes, they need to be parsed manually prior to calling the function.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/domain_conf.c | 60 ++++++++++++++++++++++++++++++++++++++++++++++++ src/conf/domain_conf.h | 6 +++++ src/libvirt_private.syms | 1 + 3 files changed, 67 insertions(+)
Perhaps you forgot to include next patch that uses virDomainStorageSourceParseNew()? Because this patch does nothing more than introducing a function that is never used.
Yes. The patch that uses this function was not posted yet. I'm sending these sub-series out so that I don't have to send 50+ patches at once.
Okay. ACK then. Michal
On 03/13/2018 03:37 PM, Peter Krempa wrote:
Fixup and cleanup the disk source parsing code and make it usable in other parts of the code by exporting it.
Peter Krempa (10): conf: Remove unnecessary condition from virDomainDiskSourceFormatInternal conf: Refactor seclabel formatting in virDomainDiskSourceFormatInternal conf: Remove virDomainDiskSourceDefFormatSeclabel conf: Refactor formatting of startupPolicy in virDomainDiskSourceFormatInternal conf: disk: Separate virStorageSource formatting conf: Validate disk source configuration also for the backing store conf: Separate seclabel validation from parsing conf: Parse and validate disk source seclabels together with the source conf: Extract parsing of storage source related data conf: Add and export wrapper for parsing storage source XML
src/conf/domain_conf.c | 402 ++++++++++++++++++++++++++++------------------- src/conf/domain_conf.h | 13 ++ src/libvirt_private.syms | 2 + 3 files changed, 254 insertions(+), 163 deletions(-)
ACK to patches 1-9. Michal
participants (2)
-
Michal Privoznik -
Peter Krempa