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

[Peter Krempa]

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 at 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


-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20180529/7545bcad/attachment-0001.sig>