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

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.

> + return -1; > + > + /* disk source was dropped */ > + continue; > + } > + } > + > + return 0; > +} > + > Reviewed-by: John Ferlan <jferlan(a)redhat.com&gt; -- libvir-list mailing list libvir-list(a)redhat.com https://www.redhat.com/mailman/listinfo/libvir-list