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@xxxxxxxxxx>
> > > ---
> > > 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
Attachment:
signature.asc
Description: PGP signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
