[libvirt] [PATCH 0/4] Remove 'bootHash'
An alternative to Peter's: [PATCH 1/4] conf: domain: Remove code accessing 'bootHash' from the post-parse infrestructure <8b4627fedd199117af1ab46f5562e3838679357e.1527500017.git.pkrempa@redhat.com> Ján Tomko (3): vmx: add VIR_DOMAIN_DEF_FEATURE_NO_BOOT_ORDER conf: introduce virDomainDefCheckBootOrder conf: remove 'bootHash' completely Peter Krempa (1): conf: remove 'bootHash' from the post-parse infrastructure src/conf/domain_conf.c | 209 +++++++++++++++------------ src/conf/domain_conf.h | 1 + src/vmx/vmx.c | 3 +- tests/qemuargv2xmldata/nomachine-aarch64.xml | 1 + tests/qemuargv2xmldata/nomachine-ppc64.xml | 1 + tests/qemuargv2xmldata/nomachine-x86_64.xml | 1 + tests/sexpr2xmldata/sexpr2xml-fv-kernel.xml | 1 + 7 files changed, 120 insertions(+), 97 deletions(-) -- 2.16.1
Further patches will introduce validation and a default setting of def->os.bootDevs in postParse. Introduce a feature flag to opt out of this and set it in the vmx driver. Signed-off-by: Ján Tomko <jtomko@redhat.com> --- src/conf/domain_conf.h | 1 + src/vmx/vmx.c | 3 ++- 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index b7e52a1e03..e10206b358 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -2557,6 +2557,7 @@ typedef enum { VIR_DOMAIN_DEF_FEATURE_NAME_SLASH = (1 << 3), VIR_DOMAIN_DEF_FEATURE_INDIVIDUAL_VCPUS = (1 << 4), VIR_DOMAIN_DEF_FEATURE_USER_ALIAS = (1 << 5), + VIR_DOMAIN_DEF_FEATURE_NO_BOOT_ORDER = (1 << 6), } virDomainDefFeatures; diff --git a/src/vmx/vmx.c b/src/vmx/vmx.c index df6a58a474..bdc27b15b0 100644 --- a/src/vmx/vmx.c +++ b/src/vmx/vmx.c @@ -550,7 +550,8 @@ static virDomainDefParserConfig virVMXDomainDefParserConfig = { .devicesPostParseCallback = virVMXDomainDevicesDefPostParse, .domainPostParseCallback = virVMXDomainDefPostParse, .features = (VIR_DOMAIN_DEF_FEATURE_WIDE_SCSI | - VIR_DOMAIN_DEF_FEATURE_NAME_SLASH), + VIR_DOMAIN_DEF_FEATURE_NAME_SLASH | + VIR_DOMAIN_DEF_FEATURE_NO_BOOT_ORDER), }; struct virVMXDomainDefNamespaceData { -- 2.16.1
On Mon, May 28, 2018 at 15:54:02 +0200, Ján Tomko wrote:
Further patches will introduce validation and a default setting of def->os.bootDevs in postParse.
Introduce a feature flag to opt out of this and set it in the vmx driver.
This does not clarify in any way why it is required.
Signed-off-by: Ján Tomko <jtomko@redhat.com> --- src/conf/domain_conf.h | 1 + src/vmx/vmx.c | 3 ++- 2 files changed, 3 insertions(+), 1 deletion(-)
On Tue, May 29, 2018 at 09:30:05AM +0200, Peter Krempa wrote:
On Mon, May 28, 2018 at 15:54:02 +0200, Ján Tomko wrote:
Further patches will introduce validation and a default setting of def->os.bootDevs in postParse.
Introduce a feature flag to opt out of this and set it in the vmx driver.
This does not clarify in any way why it is required.
Introduce a feature flag to opt out of this and set it in the vmx driver, otherwise we would be adding it <boot dev='hd'/> into every vmx config despite having no way to change it. (Alternatively, if booting from hard-drive is the default, we can just leave it in because none of the vmx code even touches bootDevs, so it will be safely ignored) Jano
Signed-off-by: Ján Tomko <jtomko@redhat.com> --- src/conf/domain_conf.h | 1 + src/vmx/vmx.c | 3 ++- 2 files changed, 3 insertions(+), 1 deletion(-)
On Tue, May 29, 2018 at 09:48:51 +0200, Ján Tomko wrote:
On Tue, May 29, 2018 at 09:30:05AM +0200, Peter Krempa wrote:
On Mon, May 28, 2018 at 15:54:02 +0200, Ján Tomko wrote:
Further patches will introduce validation and a default setting of def->os.bootDevs in postParse.
Introduce a feature flag to opt out of this and set it in the vmx driver.
This does not clarify in any way why it is required.
Introduce a feature flag to opt out of this and set it in the vmx driver, otherwise we would be adding it <boot dev='hd'/> into every vmx config despite having no way to change it.
ACK to the patch if you add this wording.
(Alternatively, if booting from hard-drive is the default, we can just leave it in because none of the vmx code even touches bootDevs, so it will be safely ignored)
It very well might be the default. The capability can easily be deleted later.
Jano
Signed-off-by: Ján Tomko <jtomko@redhat.com> --- src/conf/domain_conf.h | 1 + src/vmx/vmx.c | 3 ++- 2 files changed, 3 insertions(+), 1 deletion(-)
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Move the check for boot elements into a separate function and remove its dependency on the parser-supplied bootHash table. Reconstructing the hash table from the domain definition effectively duplicates the check for duplicate boot order values, also present in virDomainDeviceBootParseXML. Signed-off-by: Ján Tomko <jtomko@redhat.com> --- src/conf/domain_conf.c | 89 +++++++++++++++++++++++----- tests/qemuargv2xmldata/nomachine-aarch64.xml | 1 + tests/qemuargv2xmldata/nomachine-ppc64.xml | 1 + tests/qemuargv2xmldata/nomachine-x86_64.xml | 1 + tests/sexpr2xmldata/sexpr2xml-fv-kernel.xml | 1 + 5 files changed, 78 insertions(+), 15 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index d6ac47c629..f087a3680f 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -4939,10 +4939,79 @@ virDomainDefPostParseCPU(virDomainDefPtr def) } +static int +virDomainDefCollectBootOrder(virDomainDefPtr def ATTRIBUTE_UNUSED, + virDomainDeviceDefPtr dev ATTRIBUTE_UNUSED, + virDomainDeviceInfoPtr info, + void *data) +{ + virHashTablePtr bootHash = data; + char *order = NULL; + int ret = -1; + + if (info->bootIndex == 0) + return 0; + + if (virAsprintf(&order, "%u", info->bootIndex) < 0) + goto cleanup; + + if (virHashLookup(bootHash, order)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("boot order '%s' used for more than one device"), + order); + goto cleanup; + } + + if (virHashAddEntry(bootHash, order, (void *) 1) < 0) + goto cleanup; + + ret = 0; + + cleanup: + VIR_FREE(order); + return ret; +} + + +static int +virDomainDefCheckBootOrder(virDomainDefPtr def) +{ + virHashTablePtr bootHash = NULL; + int ret = -1; + + if (def->os.type != VIR_DOMAIN_OSTYPE_HVM) + return 0; + + if (!(bootHash = virHashCreate(5, NULL))) + goto cleanup; + + if (virDomainDeviceInfoIterate(def, virDomainDefCollectBootOrder, bootHash) < 0) + goto cleanup; + + if (def->os.nBootDevs > 0 && virHashSize(bootHash) > 0) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("per-device boot elements cannot be used" + " together with os/boot elements")); + goto cleanup; + } + + if (def->os.nBootDevs == 0 && virHashSize(bootHash) == 0) { + def->os.nBootDevs = 1; + def->os.bootDevs[0] = VIR_DOMAIN_BOOT_DISK; + } + + ret = 0; + + cleanup: + virHashFree(bootHash); + return ret; +} + + static int virDomainDefPostParseCommon(virDomainDefPtr def, struct virDomainDefPostParseDeviceIteratorData *data, - virHashTablePtr bootHash) + virHashTablePtr bootHash ATTRIBUTE_UNUSED) { size_t i; @@ -4953,20 +5022,6 @@ virDomainDefPostParseCommon(virDomainDefPtr def, return -1; } - if (def->os.type == VIR_DOMAIN_OSTYPE_HVM && bootHash) { - if (def->os.nBootDevs > 0 && virHashSize(bootHash) > 0) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("per-device boot elements cannot be used" - " together with os/boot elements")); - return -1; - } - - if (def->os.nBootDevs == 0 && virHashSize(bootHash) == 0) { - def->os.nBootDevs = 1; - def->os.bootDevs[0] = VIR_DOMAIN_BOOT_DISK; - } - } - if (virDomainVcpuDefPostParse(def) < 0) return -1; @@ -4979,6 +5034,10 @@ virDomainDefPostParseCommon(virDomainDefPtr def, if (virDomainDefRejectDuplicatePanics(def) < 0) return -1; + if (!(data->xmlopt->config.features & VIR_DOMAIN_DEF_FEATURE_NO_BOOT_ORDER) && + virDomainDefCheckBootOrder(def) < 0) + return -1; + if (virDomainDefPostParseTimer(def) < 0) return -1; diff --git a/tests/qemuargv2xmldata/nomachine-aarch64.xml b/tests/qemuargv2xmldata/nomachine-aarch64.xml index eb8f9db803..9492423389 100644 --- a/tests/qemuargv2xmldata/nomachine-aarch64.xml +++ b/tests/qemuargv2xmldata/nomachine-aarch64.xml @@ -6,6 +6,7 @@ <vcpu placement='static'>1</vcpu> <os> <type arch='aarch64' machine='virt'>hvm</type> + <boot dev='hd'/> </os> <features> <gic version='2'/> diff --git a/tests/qemuargv2xmldata/nomachine-ppc64.xml b/tests/qemuargv2xmldata/nomachine-ppc64.xml index 439f9e9ac6..1f15a950e3 100644 --- a/tests/qemuargv2xmldata/nomachine-ppc64.xml +++ b/tests/qemuargv2xmldata/nomachine-ppc64.xml @@ -6,6 +6,7 @@ <vcpu placement='static'>1</vcpu> <os> <type arch='ppc64' machine='pseries'>hvm</type> + <boot dev='hd'/> </os> <clock offset='utc'/> <on_poweroff>destroy</on_poweroff> diff --git a/tests/qemuargv2xmldata/nomachine-x86_64.xml b/tests/qemuargv2xmldata/nomachine-x86_64.xml index 71a36f0833..33cde4c55a 100644 --- a/tests/qemuargv2xmldata/nomachine-x86_64.xml +++ b/tests/qemuargv2xmldata/nomachine-x86_64.xml @@ -6,6 +6,7 @@ <vcpu placement='static'>1</vcpu> <os> <type arch='x86_64' machine='pc-0.11'>hvm</type> + <boot dev='hd'/> </os> <features> <acpi/> diff --git a/tests/sexpr2xmldata/sexpr2xml-fv-kernel.xml b/tests/sexpr2xmldata/sexpr2xml-fv-kernel.xml index afb9030681..a3d54ae3c1 100644 --- a/tests/sexpr2xmldata/sexpr2xml-fv-kernel.xml +++ b/tests/sexpr2xmldata/sexpr2xml-fv-kernel.xml @@ -10,6 +10,7 @@ <kernel>/var/lib/xen/vmlinuz.2Dn2YT</kernel> <initrd>/var/lib/xen/initrd.img.0u-Vhq</initrd> <cmdline> method=http://download.fedora.devel.redhat.com/pub/fedora/linux/core/test/5.91/x86_... </cmdline> + <boot dev='hd'/> </os> <clock offset='variable' adjustment='0' basis='utc'/> <on_poweroff>destroy</on_poweroff> -- 2.16.1
On Mon, May 28, 2018 at 15:54:03 +0200, Ján Tomko wrote:
Move the check for boot elements into a separate function and remove its dependency on the parser-supplied bootHash table.
Reconstructing the hash table from the domain definition effectively duplicates the check for duplicate boot order values, also present in virDomainDeviceBootParseXML.
So the semantical difference is that places that call into the post-parse infrastructure which construct the domain definition object by other means that parsing XML will get the same treatment as when XML is parsed.
Signed-off-by: Ján Tomko <jtomko@redhat.com> --- src/conf/domain_conf.c | 89 +++++++++++++++++++++++----- tests/qemuargv2xmldata/nomachine-aarch64.xml | 1 + tests/qemuargv2xmldata/nomachine-ppc64.xml | 1 + tests/qemuargv2xmldata/nomachine-x86_64.xml | 1 + tests/sexpr2xmldata/sexpr2xml-fv-kernel.xml | 1 + 5 files changed, 78 insertions(+), 15 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index d6ac47c629..f087a3680f 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -4939,10 +4939,79 @@ virDomainDefPostParseCPU(virDomainDefPtr def)
[...]
+ + +static int +virDomainDefCheckBootOrder(virDomainDefPtr def)
[1]
+{ + virHashTablePtr bootHash = NULL; + int ret = -1; + + if (def->os.type != VIR_DOMAIN_OSTYPE_HVM) + return 0;
Please do this check outside of this function. If this function is invoked it should o it's job, especially since you also have other conditions when to invoke it outside.
+ + if (!(bootHash = virHashCreate(5, NULL))) + goto cleanup; + + if (virDomainDeviceInfoIterate(def, virDomainDefCollectBootOrder, bootHash) < 0) + goto cleanup; + + if (def->os.nBootDevs > 0 && virHashSize(bootHash) > 0) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("per-device boot elements cannot be used" + " together with os/boot elements")); + goto cleanup; + } + + if (def->os.nBootDevs == 0 && virHashSize(bootHash) == 0) { + def->os.nBootDevs = 1; + def->os.bootDevs[0] = VIR_DOMAIN_BOOT_DISK;
[1] this in contrast to the name of the function modifies stuff ...
+ } + + ret = 0; + + cleanup: + virHashFree(bootHash); + return ret; +} + +
[...]
@@ -4979,6 +5034,10 @@ virDomainDefPostParseCommon(virDomainDefPtr def, if (virDomainDefRejectDuplicatePanics(def) < 0) return -1;
+ if (!(data->xmlopt->config.features & VIR_DOMAIN_DEF_FEATURE_NO_BOOT_ORDER) && + virDomainDefCheckBootOrder(def) < 0)
Add the condition for checking HVM here.
+ return -1; + if (virDomainDefPostParseTimer(def) < 0) return -1;
[...]
diff --git a/tests/sexpr2xmldata/sexpr2xml-fv-kernel.xml b/tests/sexpr2xmldata/sexpr2xml-fv-kernel.xml index afb9030681..a3d54ae3c1 100644 --- a/tests/sexpr2xmldata/sexpr2xml-fv-kernel.xml +++ b/tests/sexpr2xmldata/sexpr2xml-fv-kernel.xml @@ -10,6 +10,7 @@ <kernel>/var/lib/xen/vmlinuz.2Dn2YT</kernel> <initrd>/var/lib/xen/initrd.img.0u-Vhq</initrd> <cmdline> method=http://download.fedora.devel.redhat.com/pub/fedora/linux/core/test/5.91/x86_... </cmdline> + <boot dev='hd'/>
This looks wrong here since we do have the 'kernel' and 'initrd' elements. Given that it's caused by the semantic difference I think it's okay.
</os> <clock offset='variable' adjustment='0' basis='utc'/> <on_poweroff>destroy</on_poweroff>
ACK if you move out the condition and note the semantic impact in the commit message.
On Tue, May 29, 2018 at 09:26:51AM +0200, Peter Krempa wrote:
On Mon, May 28, 2018 at 15:54:03 +0200, Ján Tomko wrote:
Move the check for boot elements into a separate function and remove its dependency on the parser-supplied bootHash table.
Reconstructing the hash table from the domain definition effectively duplicates the check for duplicate boot order values, also present in virDomainDeviceBootParseXML.
How about: Now it will also be run on domains created by other means than XML parsing, since it will be run even for code paths that did not supply the bootHash table before.
So the semantical difference is that places that call into the post-parse infrastructure which construct the domain definition object by other means that parsing XML will get the same treatment as when XML is parsed.
Signed-off-by: Ján Tomko <jtomko@redhat.com> --- src/conf/domain_conf.c | 89 +++++++++++++++++++++++----- tests/qemuargv2xmldata/nomachine-aarch64.xml | 1 + tests/qemuargv2xmldata/nomachine-ppc64.xml | 1 + tests/qemuargv2xmldata/nomachine-x86_64.xml | 1 + tests/sexpr2xmldata/sexpr2xml-fv-kernel.xml | 1 + 5 files changed, 78 insertions(+), 15 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index d6ac47c629..f087a3680f 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -4939,10 +4939,79 @@ virDomainDefPostParseCPU(virDomainDefPtr def)
[...]
+ + +static int +virDomainDefCheckBootOrder(virDomainDefPtr def)
[1]
+{ + virHashTablePtr bootHash = NULL; + int ret = -1; + + if (def->os.type != VIR_DOMAIN_OSTYPE_HVM) + return 0;
Please do this check outside of this function. If this function is invoked it should o it's job, especially since you also have other conditions when to invoke it outside.
+ + if (!(bootHash = virHashCreate(5, NULL))) + goto cleanup; + + if (virDomainDeviceInfoIterate(def, virDomainDefCollectBootOrder, bootHash) < 0) + goto cleanup; + + if (def->os.nBootDevs > 0 && virHashSize(bootHash) > 0) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("per-device boot elements cannot be used" + " together with os/boot elements")); + goto cleanup; + } + + if (def->os.nBootDevs == 0 && virHashSize(bootHash) == 0) { + def->os.nBootDevs = 1; + def->os.bootDevs[0] = VIR_DOMAIN_BOOT_DISK;
[1] this in contrast to the name of the function modifies stuff ...
I can rename it to virDomainDefBootOrderPostParse Jano
On Tue, May 29, 2018 at 09:55:18 +0200, Ján Tomko wrote:
On Tue, May 29, 2018 at 09:26:51AM +0200, Peter Krempa wrote:
On Mon, May 28, 2018 at 15:54:03 +0200, Ján Tomko wrote:
Move the check for boot elements into a separate function and remove its dependency on the parser-supplied bootHash table.
Reconstructing the hash table from the domain definition effectively duplicates the check for duplicate boot order values, also present in virDomainDeviceBootParseXML.
How about: Now it will also be run on domains created by other means than XML parsing, since it will be run even for code paths that did not supply the bootHash table before.
Sounds goog to me.
So the semantical difference is that places that call into the post-parse infrastructure which construct the domain definition object by other means that parsing XML will get the same treatment as when XML is parsed.
Signed-off-by: Ján Tomko <jtomko@redhat.com> --- src/conf/domain_conf.c | 89 +++++++++++++++++++++++----- tests/qemuargv2xmldata/nomachine-aarch64.xml | 1 + tests/qemuargv2xmldata/nomachine-ppc64.xml | 1 + tests/qemuargv2xmldata/nomachine-x86_64.xml | 1 + tests/sexpr2xmldata/sexpr2xml-fv-kernel.xml | 1 + 5 files changed, 78 insertions(+), 15 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index d6ac47c629..f087a3680f 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -4939,10 +4939,79 @@ virDomainDefPostParseCPU(virDomainDefPtr def)
[...]
+ + +static int +virDomainDefCheckBootOrder(virDomainDefPtr def)
[1]
+{ + virHashTablePtr bootHash = NULL; + int ret = -1; + + if (def->os.type != VIR_DOMAIN_OSTYPE_HVM) + return 0;
Please do this check outside of this function. If this function is invoked it should o it's job, especially since you also have other conditions when to invoke it outside.
+ + if (!(bootHash = virHashCreate(5, NULL))) + goto cleanup; + + if (virDomainDeviceInfoIterate(def, virDomainDefCollectBootOrder, bootHash) < 0) + goto cleanup; + + if (def->os.nBootDevs > 0 && virHashSize(bootHash) > 0) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("per-device boot elements cannot be used" + " together with os/boot elements")); + goto cleanup; + } + + if (def->os.nBootDevs == 0 && virHashSize(bootHash) == 0) { + def->os.nBootDevs = 1; + def->os.bootDevs[0] = VIR_DOMAIN_BOOT_DISK;
[1] this in contrast to the name of the function modifies stuff ...
I can rename it to virDomainDefBootOrderPostParse
ACK to that
Jano
From: Peter Krempa <pkrempa@redhat.com> As the function signature of virDomainDefPostParseInternal does not differ from virDomainDefPostParse now, the wrapper can be dropped. Signed-off-by: Peter Krempa <pkrempa@redhat.com> Signed-off-by: Ján Tomko <jtomko@redhat.com> --- src/conf/domain_conf.c | 33 +++++++++------------------------ 1 file changed, 9 insertions(+), 24 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index f087a3680f..6076424bc6 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -5010,8 +5010,7 @@ virDomainDefCheckBootOrder(virDomainDefPtr def) static int virDomainDefPostParseCommon(virDomainDefPtr def, - struct virDomainDefPostParseDeviceIteratorData *data, - virHashTablePtr bootHash ATTRIBUTE_UNUSED) + struct virDomainDefPostParseDeviceIteratorData *data) { size_t i; @@ -5118,13 +5117,12 @@ virDomainDefPostParseCheckFailure(virDomainDefPtr def, } -static int -virDomainDefPostParseInternal(virDomainDefPtr def, - virCapsPtr caps, - unsigned int parseFlags, - virDomainXMLOptionPtr xmlopt, - void *parseOpaque, - virHashTablePtr bootHash) +int +virDomainDefPostParse(virDomainDefPtr def, + virCapsPtr caps, + unsigned int parseFlags, + virDomainXMLOptionPtr xmlopt, + void *parseOpaque) { int ret = -1; bool localParseOpaque = false; @@ -5180,7 +5178,7 @@ virDomainDefPostParseInternal(virDomainDefPtr def, if (virDomainDefPostParseCheckFailure(def, parseFlags, ret) < 0) goto cleanup; - if ((ret = virDomainDefPostParseCommon(def, &data, bootHash)) < 0) + if ((ret = virDomainDefPostParseCommon(def, &data)) < 0) goto cleanup; if (xmlopt->config.assignAddressesCallback) { @@ -5207,18 +5205,6 @@ virDomainDefPostParseInternal(virDomainDefPtr def, } -int -virDomainDefPostParse(virDomainDefPtr def, - virCapsPtr caps, - unsigned int parseFlags, - virDomainXMLOptionPtr xmlopt, - void *parseOpaque) -{ - return virDomainDefPostParseInternal(def, caps, parseFlags, xmlopt, - parseOpaque, NULL); -} - - /** * virDomainDiskAddressDiskBusCompatibility: * @bus: disk bus type @@ -20562,8 +20548,7 @@ virDomainDefParseXML(xmlDocPtr xml, goto error; /* callback to fill driver specific domain aspects */ - if (virDomainDefPostParseInternal(def, caps, flags, xmlopt, parseOpaque, - bootHash) < 0) + if (virDomainDefPostParse(def, caps, flags, xmlopt, parseOpaque) < 0) goto error; /* valdiate configuration */ -- 2.16.1
On Mon, May 28, 2018 at 15:54:04 +0200, Ján Tomko wrote:
From: Peter Krempa <pkrempa@redhat.com>
As the function signature of virDomainDefPostParseInternal does not differ from virDomainDefPostParse now, the wrapper can be dropped.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> Signed-off-by: Ján Tomko <jtomko@redhat.com> --- src/conf/domain_conf.c | 33 +++++++++------------------------ 1 file changed, 9 insertions(+), 24 deletions(-)
Implicit ACK. Also please push the rest of the patches from my series which depend on this when you'll be pushing this.
On Tue, May 29, 2018 at 09:27:53AM +0200, Peter Krempa wrote:
On Mon, May 28, 2018 at 15:54:04 +0200, Ján Tomko wrote:
From: Peter Krempa <pkrempa@redhat.com>
As the function signature of virDomainDefPostParseInternal does not differ from virDomainDefPostParse now, the wrapper can be dropped.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> Signed-off-by: Ján Tomko <jtomko@redhat.com> --- src/conf/domain_conf.c | 33 +++++++++------------------------ 1 file changed, 9 insertions(+), 24 deletions(-)
Implicit ACK. Also please push the rest of the patches from my series which depend on this when you'll be pushing this.
Done. Jano
Its only use is now to check for duplicate boot order values, which is now also done in virDomainDefPostParseCommon. Remove it completely. Signed-off-by: Ján Tomko <jtomko@redhat.com> --- src/conf/domain_conf.c | 89 ++++++++++++++++++-------------------------------- 1 file changed, 31 insertions(+), 58 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 6076424bc6..c150b7d7d4 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -6587,8 +6587,7 @@ virDomainDeviceUSBMasterParseXML(xmlNodePtr node, static int virDomainDeviceBootParseXML(xmlNodePtr node, - virDomainDeviceInfoPtr info, - virHashTablePtr bootHash) + virDomainDeviceInfoPtr info) { char *order; char *loadparm = NULL; @@ -6608,18 +6607,6 @@ virDomainDeviceBootParseXML(xmlNodePtr node, goto cleanup; } - if (bootHash) { - if (virHashLookup(bootHash, order)) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("boot order '%s' used for more than one device"), - order); - goto cleanup; - } - - if (virHashAddEntry(bootHash, order, (void *) 1) < 0) - goto cleanup; - } - loadparm = virXMLPropString(node, "loadparm"); if (loadparm) { if (virStringToUpper(&info->loadparm, loadparm) != 1) { @@ -6817,7 +6804,6 @@ virDomainDeviceAliasIsUserAlias(const char *aliasStr) static int virDomainDeviceInfoParseXML(virDomainXMLOptionPtr xmlopt ATTRIBUTE_UNUSED, xmlNodePtr node, - virHashTablePtr bootHash, virDomainDeviceInfoPtr info, unsigned int flags) { @@ -6877,7 +6863,7 @@ virDomainDeviceInfoParseXML(virDomainXMLOptionPtr xmlopt ATTRIBUTE_UNUSED, } if (boot) { - if (virDomainDeviceBootParseXML(boot, info, bootHash)) + if (virDomainDeviceBootParseXML(boot, info)) goto cleanup; } @@ -9402,7 +9388,6 @@ static virDomainDiskDefPtr virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt, xmlNodePtr node, xmlXPathContextPtr ctxt, - virHashTablePtr bootHash, virSecurityLabelDefPtr* vmSeclabels, int nvmSeclabels, unsigned int flags) @@ -9769,7 +9754,7 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt, } def->info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI; } else { - if (virDomainDeviceInfoParseXML(xmlopt, node, bootHash, &def->info, + if (virDomainDeviceInfoParseXML(xmlopt, node, &def->info, flags | VIR_DOMAIN_DEF_PARSE_ALLOW_BOOT) < 0) goto error; } @@ -10260,7 +10245,7 @@ virDomainControllerDefParseXML(virDomainXMLOptionPtr xmlopt, if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_USB && def->model == VIR_DOMAIN_CONTROLLER_MODEL_USB_NONE) { VIR_DEBUG("Ignoring device address for none model usb controller"); - } else if (virDomainDeviceInfoParseXML(xmlopt, node, NULL, + } else if (virDomainDeviceInfoParseXML(xmlopt, node, &def->info, flags) < 0) { goto error; } @@ -10648,7 +10633,7 @@ virDomainFSDefParseXML(virDomainXMLOptionPtr xmlopt, def->dst = target; target = NULL; - if (virDomainDeviceInfoParseXML(xmlopt, node, NULL, &def->info, flags) < 0) + if (virDomainDeviceInfoParseXML(xmlopt, node, &def->info, flags) < 0) goto error; cleanup: @@ -10938,7 +10923,6 @@ static virDomainNetDefPtr virDomainNetDefParseXML(virDomainXMLOptionPtr xmlopt, xmlNodePtr node, xmlXPathContextPtr ctxt, - virHashTablePtr bootHash, char *prefix, unsigned int flags) { @@ -11226,7 +11210,7 @@ virDomainNetDefParseXML(virDomainXMLOptionPtr xmlopt, } def->info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI; } else { - if (virDomainDeviceInfoParseXML(xmlopt, node, bootHash, &def->info, + if (virDomainDeviceInfoParseXML(xmlopt, node, &def->info, flags | VIR_DOMAIN_DEF_PARSE_ALLOW_BOOT | VIR_DOMAIN_DEF_PARSE_ALLOW_ROM) < 0) goto error; @@ -12525,7 +12509,7 @@ virDomainChrDefParseXML(virDomainXMLOptionPtr xmlopt, if (def->deviceType == VIR_DOMAIN_CHR_DEVICE_TYPE_CHANNEL && def->targetType == VIR_DOMAIN_CHR_CHANNEL_TARGET_TYPE_GUESTFWD) { VIR_DEBUG("Ignoring device address for gustfwd channel"); - } else if (virDomainDeviceInfoParseXML(xmlopt, node, NULL, + } else if (virDomainDeviceInfoParseXML(xmlopt, node, &def->info, flags) < 0) { goto error; } @@ -12663,7 +12647,7 @@ virDomainSmartcardDefParseXML(virDomainXMLOptionPtr xmlopt, goto error; } - if (virDomainDeviceInfoParseXML(xmlopt, node, NULL, &def->info, flags) < 0) + if (virDomainDeviceInfoParseXML(xmlopt, node, &def->info, flags) < 0) goto error; if (def->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE && def->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCID) { @@ -12764,7 +12748,7 @@ virDomainTPMDefParseXML(virDomainXMLOptionPtr xmlopt, goto error; } - if (virDomainDeviceInfoParseXML(xmlopt, node, NULL, &def->info, flags) < 0) + if (virDomainDeviceInfoParseXML(xmlopt, node, &def->info, flags) < 0) goto error; cleanup: @@ -12793,7 +12777,7 @@ virDomainPanicDefParseXML(virDomainXMLOptionPtr xmlopt, if (VIR_ALLOC(panic) < 0) return NULL; - if (virDomainDeviceInfoParseXML(xmlopt, node, NULL, + if (virDomainDeviceInfoParseXML(xmlopt, node, &panic->info, flags) < 0) goto error; @@ -12929,7 +12913,7 @@ virDomainInputDefParseXML(virDomainXMLOptionPtr xmlopt, } } - if (virDomainDeviceInfoParseXML(xmlopt, node, NULL, &def->info, flags) < 0) + if (virDomainDeviceInfoParseXML(xmlopt, node, &def->info, flags) < 0) goto error; if (def->bus == VIR_DOMAIN_INPUT_BUS_USB && @@ -12993,7 +12977,7 @@ virDomainHubDefParseXML(virDomainXMLOptionPtr xmlopt, goto error; } - if (virDomainDeviceInfoParseXML(xmlopt, node, NULL, &def->info, flags) < 0) + if (virDomainDeviceInfoParseXML(xmlopt, node, &def->info, flags) < 0) goto error; cleanup: @@ -14150,7 +14134,7 @@ virDomainSoundDefParseXML(virDomainXMLOptionPtr xmlopt, } } - if (virDomainDeviceInfoParseXML(xmlopt, node, NULL, &def->info, flags) < 0) + if (virDomainDeviceInfoParseXML(xmlopt, node, &def->info, flags) < 0) goto error; cleanup: @@ -14204,7 +14188,7 @@ virDomainWatchdogDefParseXML(virDomainXMLOptionPtr xmlopt, } } - if (virDomainDeviceInfoParseXML(xmlopt, node, NULL, &def->info, flags) < 0) + if (virDomainDeviceInfoParseXML(xmlopt, node, &def->info, flags) < 0) goto error; cleanup: @@ -14316,7 +14300,7 @@ virDomainRNGDefParseXML(virDomainXMLOptionPtr xmlopt, break; } - if (virDomainDeviceInfoParseXML(xmlopt, node, NULL, &def->info, flags) < 0) + if (virDomainDeviceInfoParseXML(xmlopt, node, &def->info, flags) < 0) goto error; if (virDomainVirtioOptionsParseXML(virXPathNode("./driver", ctxt), @@ -14386,7 +14370,7 @@ virDomainMemballoonDefParseXML(virDomainXMLOptionPtr xmlopt, if (def->model == VIR_DOMAIN_MEMBALLOON_MODEL_NONE) VIR_DEBUG("Ignoring device address for none model Memballoon"); - else if (virDomainDeviceInfoParseXML(xmlopt, node, NULL, + else if (virDomainDeviceInfoParseXML(xmlopt, node, &def->info, flags) < 0) goto error; @@ -14417,7 +14401,7 @@ virDomainNVRAMDefParseXML(virDomainXMLOptionPtr xmlopt, if (VIR_ALLOC(def) < 0) return NULL; - if (virDomainDeviceInfoParseXML(xmlopt, node, NULL, &def->info, flags) < 0) + if (virDomainDeviceInfoParseXML(xmlopt, node, &def->info, flags) < 0) goto error; return def; @@ -14513,7 +14497,7 @@ virDomainShmemDefParseXML(virDomainXMLOptionPtr xmlopt, goto cleanup; } - if (virDomainDeviceInfoParseXML(xmlopt, node, NULL, &def->info, flags) < 0) + if (virDomainDeviceInfoParseXML(xmlopt, node, &def->info, flags) < 0) goto cleanup; @@ -15167,7 +15151,7 @@ virDomainVideoDefParseXML(virDomainXMLOptionPtr xmlopt, } } - if (virDomainDeviceInfoParseXML(xmlopt, node, NULL, &def->info, flags) < 0) + if (virDomainDeviceInfoParseXML(xmlopt, node, &def->info, flags) < 0) goto error; def->driver = virDomainVideoDriverDefParseXML(node); @@ -15194,7 +15178,6 @@ static virDomainHostdevDefPtr virDomainHostdevDefParseXML(virDomainXMLOptionPtr xmlopt, xmlNodePtr node, xmlXPathContextPtr ctxt, - virHashTablePtr bootHash, unsigned int flags) { virDomainHostdevDefPtr def; @@ -15235,7 +15218,7 @@ virDomainHostdevDefParseXML(virDomainXMLOptionPtr xmlopt, } if (def->info->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE) { - if (virDomainDeviceInfoParseXML(xmlopt, node, bootHash, def->info, + if (virDomainDeviceInfoParseXML(xmlopt, node, def->info, flags | VIR_DOMAIN_DEF_PARSE_ALLOW_BOOT | VIR_DOMAIN_DEF_PARSE_ALLOW_ROM) < 0) goto error; @@ -15275,7 +15258,6 @@ static virDomainRedirdevDefPtr virDomainRedirdevDefParseXML(virDomainXMLOptionPtr xmlopt, xmlNodePtr node, xmlXPathContextPtr ctxt, - virHashTablePtr bootHash, unsigned int flags) { xmlNodePtr cur; @@ -15322,7 +15304,7 @@ virDomainRedirdevDefParseXML(virDomainXMLOptionPtr xmlopt, if (def->source->type == VIR_DOMAIN_CHR_TYPE_SPICEVMC) def->source->data.spicevmc = VIR_DOMAIN_CHR_SPICEVMC_USBREDIR; - if (virDomainDeviceInfoParseXML(xmlopt, node, bootHash, &def->info, + if (virDomainDeviceInfoParseXML(xmlopt, node, &def->info, flags | VIR_DOMAIN_DEF_PARSE_ALLOW_BOOT) < 0) goto error; @@ -15801,7 +15783,7 @@ virDomainMemoryDefParseXML(virDomainXMLOptionPtr xmlopt, goto error; if (virDomainDeviceInfoParseXML(xmlopt, memdevNode, - NULL, &def->info, flags) < 0) + &def->info, flags) < 0) goto error; ctxt->node = save; @@ -15931,7 +15913,7 @@ virDomainDeviceDefParse(const char *xmlStr, switch ((virDomainDeviceType) dev->type) { case VIR_DOMAIN_DEVICE_DISK: if (!(dev->data.disk = virDomainDiskDefParseXML(xmlopt, node, ctxt, - NULL, def->seclabels, + def->seclabels, def->nseclabels, flags))) goto error; @@ -15947,7 +15929,7 @@ virDomainDeviceDefParse(const char *xmlStr, case VIR_DOMAIN_DEVICE_NET: netprefix = caps->host.netprefix; if (!(dev->data.net = virDomainNetDefParseXML(xmlopt, node, ctxt, - NULL, netprefix, flags))) + netprefix, flags))) goto error; break; case VIR_DOMAIN_DEVICE_INPUT: @@ -15972,7 +15954,7 @@ virDomainDeviceDefParse(const char *xmlStr, break; case VIR_DOMAIN_DEVICE_HOSTDEV: if (!(dev->data.hostdev = virDomainHostdevDefParseXML(xmlopt, node, - ctxt, NULL, + ctxt, flags))) goto error; break; @@ -15991,7 +15973,7 @@ virDomainDeviceDefParse(const char *xmlStr, break; case VIR_DOMAIN_DEVICE_REDIRDEV: if (!(dev->data.redirdev = virDomainRedirdevDefParseXML(xmlopt, node, - ctxt, NULL, flags))) + ctxt, flags))) goto error; break; case VIR_DOMAIN_DEVICE_RNG: @@ -16098,7 +16080,7 @@ virDomainDiskDefParse(const char *xmlStr, } disk = virDomainDiskDefParseXML(xmlopt, ctxt->node, ctxt, - NULL, seclabels, nseclabels, flags); + seclabels, nseclabels, flags); cleanup: xmlFreeDoc(xml); @@ -18478,8 +18460,7 @@ virDomainVcpuParse(virDomainDefPtr def, static int virDomainDefParseBootOptions(virDomainDefPtr def, - xmlXPathContextPtr ctxt, - virHashTablePtr *bootHash) + xmlXPathContextPtr ctxt) { xmlNodePtr *nodes = NULL; char *tmp = NULL; @@ -18611,8 +18592,6 @@ virDomainDefParseBootOptions(virDomainDefPtr def, if (virDomainDefParseBootXML(ctxt, def) < 0) goto error; - if (!(*bootHash = virHashCreate(5, NULL))) - goto error; } ret = 0; @@ -18829,7 +18808,6 @@ virDomainDefParseXML(xmlDocPtr xml, long id = -1; virDomainDefPtr def; bool uuid_generated = false; - virHashTablePtr bootHash = NULL; bool usb_none = false; bool usb_other = false; bool usb_master = false; @@ -19874,7 +19852,7 @@ virDomainDefParseXML(xmlDocPtr xml, } VIR_FREE(nodes); - if (virDomainDefParseBootOptions(def, ctxt, &bootHash) < 0) + if (virDomainDefParseBootOptions(def, ctxt) < 0) goto error; /* analysis of the disk devices */ @@ -19888,7 +19866,6 @@ virDomainDefParseXML(xmlDocPtr xml, virDomainDiskDefPtr disk = virDomainDiskDefParseXML(xmlopt, nodes[i], ctxt, - bootHash, def->seclabels, def->nseclabels, flags); @@ -19995,7 +19972,6 @@ virDomainDefParseXML(xmlDocPtr xml, virDomainNetDefPtr net = virDomainNetDefParseXML(xmlopt, nodes[i], ctxt, - bootHash, netprefix, flags); if (!net) @@ -20237,7 +20213,7 @@ virDomainDefParseXML(xmlDocPtr xml, virDomainHostdevDefPtr hostdev; hostdev = virDomainHostdevDefParseXML(xmlopt, nodes[i], ctxt, - bootHash, flags); + flags); if (!hostdev) goto error; @@ -20381,7 +20357,7 @@ virDomainDefParseXML(xmlDocPtr xml, goto error; for (i = 0; i < n; i++) { virDomainRedirdevDefPtr redirdev = - virDomainRedirdevDefParseXML(xmlopt, nodes[i], ctxt, bootHash, flags); + virDomainRedirdevDefParseXML(xmlopt, nodes[i], ctxt, flags); if (!redirdev) goto error; @@ -20555,14 +20531,11 @@ virDomainDefParseXML(xmlDocPtr xml, if (virDomainDefValidate(def, caps, flags, xmlopt) < 0) goto error; - virHashFree(bootHash); - return def; error: VIR_FREE(tmp); VIR_FREE(nodes); - virHashFree(bootHash); virDomainDefFree(def); return NULL; } -- 2.16.1
On Mon, May 28, 2018 at 15:54:05 +0200, Ján Tomko wrote:
Its only use is now to check for duplicate boot order values, which is now also done in virDomainDefPostParseCommon.
Remove it completely.
Signed-off-by: Ján Tomko <jtomko@redhat.com> --- src/conf/domain_conf.c | 89 ++++++++++++++++++-------------------------------- 1 file changed, 31 insertions(+), 58 deletions(-)
ACK
participants (2)
-
Ján Tomko -
Peter Krempa