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&gt; > --- > 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 ...