Re: [libvirt] [PATCH 2/4] conf: introduce virDomainDefCheckBootOrder

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(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 ... I can rename it to virDomainDefBootOrderPostParse

Jano