Re: [libvirt] [PATCH] qemu: Don't assume secret provided for LUKS encryption

On Thu, Dec 22, 2016 at 09:12:43AM -0500, John Ferlan wrote: > https://bugzilla.redhat.com/show_bug.cgi?id=1405269 > > If a secret was not provided for what was determined to be a LUKS > encrypted disk (during virStorageFileGetMetadata processing when > called from qemuDomainDetermineDiskChain as a result of hotplug > attach qemuDomainAttachDeviceDiskLive), then do not attempt to > look it up (avoiding a libvirtd crash) and do not alter the format > to "luks" when adding the disk; otherwise, the device_add would > fail with a message such as: > > "unable to execute QEMU command 'device_add': Property 'scsi-hd.drive' > can't find value 'drive-scsi0-0-0-0'" > > because of assumptions that when the format=luks that libvirt would have > provided the secret to decrypt the volume. > > Access to unlock the volume will thus be left to the application. > > Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; > --- > > I could have spread things over 2 patches (one to create the helper > and one to add the extra check for nsecrets > 0), but just going with > one to fix the issue just felt cleaner (besides makes any possible > backports a bit simpler). > > NB: The "extra" !virStorageSourceIsEmpty check for the qemu_command > path is a no-op essentially. > > src/qemu/qemu_command.c | 3 +-- > src/qemu/qemu_domain.c | 15 +++++++++++++-- > src/qemu/qemu_domain.h | 3 +++ > src/qemu/qemu_hotplug.c | 3 ++- > 4 files changed, 19 insertions(+), 5 deletions(-) > ACK > diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c > index 92a2e73..6b10e63 100644 > --- a/src/qemu/qemu_hotplug.c > +++ b/src/qemu/qemu_hotplug.c > @@ -3584,7 +3584,8 @@ qemuDomainRemoveDiskDevice(virQEMUDriverPtr driver, > * can remove the luks object password too > */ > if (!virStorageSourceIsEmpty(disk->src) && disk->src->encryption && > - disk->src->encryption->format == > VIR_STORAGE_ENCRYPTION_FORMAT_LUKS) { > + disk->src->encryption->format == > VIR_STORAGE_ENCRYPTION_FORMAT_LUKS && > + disk->src->encryption->nsecrets > 0) { Why doesn't this code also use the helper?