[libvirt] [PATCH 5/7] qemu: process: Move 'volume' translation to domain prepare stage

[Peter Krempa]

On Wed, Oct 04, 2017 at 12:06:28 -0400, John Ferlan wrote:
> 
> 
> On 10/04/2017 07:42 AM, Peter Krempa wrote:
> > Introduce a new function to prepare domain disks which will also do the
> > volume source to actual disk source translation.
> > ---
> >  src/qemu/qemu_domain.c  | 10 +---------
> >  src/qemu/qemu_domain.h  |  3 +--
> >  src/qemu/qemu_process.c | 36 ++++++++++++++++++++++++++++++++----
> >  3 files changed, 34 insertions(+), 15 deletions(-)
> > 
> > diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> > index 8aa082618..bf2ce29bf 100644
> > --- a/src/qemu/qemu_domain.c
> > +++ b/src/qemu/qemu_domain.c
> > @@ -5682,8 +5682,7 @@ qemuDomainCheckDiskStartupPolicy(virQEMUDriverPtr driver,
> > 
> > 
> >  int
> > -qemuDomainCheckDiskPresence(virConnectPtr conn,
> > -                            virQEMUDriverPtr driver,
> > +qemuDomainCheckDiskPresence(virQEMUDriverPtr driver,
> >                              virDomainObjPtr vm,
> >                              unsigned int flags)
> >  {
> > @@ -5697,13 +5696,6 @@ qemuDomainCheckDiskPresence(virConnectPtr conn,
> >          virDomainDiskDefPtr disk = vm->def->disks[idx];
> >          virStorageFileFormat format = virDomainDiskGetFormat(disk);
> > 
> > -        if (virStorageTranslateDiskSourcePool(conn, vm->def->disks[idx]) < 0) {
> > -            if (pretend ||
> > -                qemuDomainCheckDiskStartupPolicy(driver, vm, idx, cold_boot) < 0)
> > -                return -1;
> > -            continue;
> > -        }
> > -
> >          if (pretend)
> >              continue;
> 
> So the reality is we don't even have to run through any of the loop if
> pretend == true as we're literally doing nothing with it.
> 
> I know, moot point since 2 patches later the whole function moves to
> being called from PrepareHost which never checked pretend anyway -
> although it perhaps could now that PrepareHost takes @flags
> 
> > 
> > diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h
> > index b3db50c2f..914f2bec9 100644
> > --- a/src/qemu/qemu_domain.h
> > +++ b/src/qemu/qemu_domain.h
> > @@ -644,8 +644,7 @@ int qemuDomainCheckDiskStartupPolicy(virQEMUDriverPtr driver,
> >                                       size_t diskIndex,
> >                                       bool cold_boot);
> > 
> > -int qemuDomainCheckDiskPresence(virConnectPtr conn,
> > -                                virQEMUDriverPtr driver,
> > +int qemuDomainCheckDiskPresence(virQEMUDriverPtr driver,
> >                                  virDomainObjPtr vm,
> >                                  unsigned int flags);
> > 
> > diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> > index dfaacbcb9..ad7c7ee81 100644
> > --- a/src/qemu/qemu_process.c
> > +++ b/src/qemu/qemu_process.c
> > @@ -5275,6 +5275,32 @@ qemuProcessPrepareDomainNUMAPlacement(virDomainObjPtr vm,
> >  }
> > 
> > 
> > +static int
> > +qemuProcessPrepareDomainStorage(virConnectPtr conn,
> > +                                virQEMUDriverPtr driver,
> > +                                virDomainObjPtr vm,
> > +                                unsigned int flags)
> > +{
> > +    size_t i;
> > +    bool cold_boot = flags & VIR_QEMU_PROCESS_START_COLD;
> > +
> > +    for (i = vm->def->ndisks; i > 0; i--) {
> > +        size_t idx = i - 1;
> > +        virDomainDiskDefPtr disk = vm->def->disks[idx];
> > +
> > +        if (virStorageTranslateDiskSourcePool(conn, disk) < 0) {
> > +            if (qemuDomainCheckDiskStartupPolicy(driver, vm, idx, cold_boot) < 0)
> 
> Missing the "pretend ||" failure option/shortcut from commit 'a2b97a8d'
> 
> I would think it would be necessary in this failure path since it's
> still eventually going to be called via/through PrepareDomain which
> seems to care about pretending...  Although make check passes, so who knows.

The condition skips dropping of the disk from the definition. Which
actually makes it hard to test whether the dropping code works (bud
necessitates setup of the volume to test it). Since neither of the tests
were present in the testsuite I think we should not special case it.

If a test wants to test that volume lookup works properly, it will need
to set them up. Otherwise the command line generator will not work.

I think of the condition as a bug in the testsuite. I'll document that
we are going to drop it.

> 
> 
> > +                return -1;
> > +
> > +            /* disk source was dropped */
> > +            continue;
> > +        }
> > +    }
> > +
> > +    return 0;
> > +}
> > +
> > Reviewed-by: John Ferlan <jferlan at redhat.com>
> 
> --
> libvir-list mailing list
> libvir-list at redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
-------------- 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/20171005/8f976bb7/attachment-0001.sig>