[libvirt] [PATCH 3/3] qemu: domain: Fix backing store terminator for non-backing local files

[Peter Krempa]

On Wed, Nov 29, 2017 at 21:29:32 -0500, John Ferlan wrote:
> 
> 
> On 11/24/2017 07:21 AM, Peter Krempa wrote:
> > Raw local files do not pass through the backing store detector and thus
> > the code did not allocate the required backing store terminator for
> > them. Previously the terminating element would be formatted into the XML
> > since the default values used for the metadata allowed that. This is a
> > regression since a693fdba0111ff which was not detected in the review.
> 
> This this is a bug fix to the bug that that patch was attempting to fix?
>  I do see it being pointed out as a comment from review that there's a
> lot of backingStore removals...  Perhaps better said - the initial patch
> was too aggressive but neglected to handle xxxx case?  I'm sure there's
> a better way to wordsmith this particular attribution.
> 
> > 
> > This patch also reverts all the changes in the test files.
> > ---
> >  src/qemu/qemu_domain.c                                               | 5 +++++
> 
> Comparing the files that changed in a693fdba0111ff...
> 
> >  .../qemuhotplugtestdomains/qemuhotplug-base-ccw-live+ccw-virtio.xml  | 1 +
> >  ...uhotplug-base-ccw-live-with-2-ccw-virtio+ccw-virtio-1-reverse.xml | 2 ++
> >  .../qemuhotplug-base-ccw-live-with-2-ccw-virtio.xml                  | 1 +
> >  ...muhotplug-base-ccw-live-with-ccw-virtio+ccw-virtio-2-explicit.xml | 1 +
> >  .../qemuhotplug-base-ccw-live-with-ccw-virtio+ccw-virtio-2.xml       | 1 +
> 
> Why does qemuhotplug-base-ccw-live-with-2-ccw-virtio.xml get the
> backingStore, but qemuhotplug-base-ccw-live-with-ccw-virtio.xml does not?
> 
> I can understand why the "added" hotplug disks have it, but it's unclear
> why the "base" file doesn't for the non "with-2" test.

This is because of the more complex test, where one device is attached
to the domain "base-ccw-live-with-ccw-virtio" and then compared to
"base-ccw-live-with-2-ccw-virtio" so that the first device can be
detached.

The device XMLs go through the hotplug code which adds the
<backingStore> element, but the domain XMLs are not initialized via our
qemuProcessPrepare.... code, thus the terminator was not added in that
code path.

> >  .../qemuhotplug-base-live+disk-scsi-wwn+disk-scsi-duplicate-wwn.xml  | 1 +
> >  tests/qemuhotplugtestdomains/qemuhotplug-base-live+disk-scsi.xml     | 1 +
> >  tests/qemuhotplugtestdomains/qemuhotplug-base-live+disk-usb.xml      | 1 +
> >  tests/qemuhotplugtestdomains/qemuhotplug-base-live+disk-virtio.xml   | 1 +
> >  .../qemuhotplug-base-without-scsi-controller-live+disk-scsi-2.xml    | 1 +
> >  11 files changed, 16 insertions(+)
> > 
> > diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> > index 0cdcb11c37..82671d99c6 100644
> > --- a/src/qemu/qemu_domain.c
> > +++ b/src/qemu/qemu_domain.c
> > @@ -6167,6 +6167,11 @@ qemuDomainDetermineDiskChain(virQEMUDriverPtr driver,
> >              goto cleanup;
> >          }
> > 
> > +        /* terminate the chain for such images as the code below would do */
> > +        if (!src->backingStore &&
> > +            VIR_ALLOC(src->backingStore) < 0)
> 
> Should be able to fit on one line
> 
> Reviewed-by: John Ferlan <jferlan at redhat.com>
> 
> I'm OK w/ this and patch 2 in during freeze - the rules are a bit grey
> when it comes to fixing problems without bz's though...

Well you may perceive it as 'grey' since there aren't any rules
regarding this. This is upstream. Bugs exist even if they are not
tracked by a bugzilla.

In this case these patches spawned from a conversation on
https://bugzilla.redhat.com/show_bug.cgi?id=1509110

I'll add it to the commit message

-------------- 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/20171130/c270686c/attachment-0001.sig>