Re: [libvirt] [PATCH 2/4] conf: introduce virDomainDefCheckBootOrder
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(a)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...
</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.
Attachments:
- signature.asc (application/pgp-signature — 833 bytes)