[libvirt] [PATCH 1/4] qemu: export disk snapshot support in capabilities

[Francesco Romani]

Hi,

thanks for the review!

----- Original Message -----
> From: "Eric Blake" <eblake at redhat.com>
> To: "Francesco Romani" <fromani at redhat.com>, libvir-list at redhat.com
> Sent: Wednesday, February 26, 2014 11:30:07 PM
> Subject: Re: [libvirt] [PATCH 1/4] qemu: export disk snapshot support in capabilities
> 
> On 01/17/2014 08:31 AM, Francesco Romani wrote:
> > This patch add an element to QEMU's capability XML, to
> 
> s/add/adds/
> 
> > show if the underlying QEMU binary supports the live disk
> > snapshotting or not.
> > This allow any client to know ahead of time if the feature
> 
> s/allow/allows/

Oops. Both fixed.

> >  docs/schemas/capability.rng  | 6 ++++++
> >  src/qemu/qemu_capabilities.c | 7 +++++++
> >  2 files changed, 13 insertions(+)
> 
> It would probably also be good to test this in
> tests/capabilityschemadata/, but maybe that happens later in the series...

Actually not. I will work on this.

> >          </optional>
> > +        <optional>
> > +          <element name='disksnapshot'>
> 
> Existing capabilities have an interesting mix of spelling: underscores:
> <os_type>, squashed: <baselabel>, camelCase: <machine maxCpus=''>.  I
> would have problaby picked <disk-snapshot> if I were writing it without
> knowledge of pre-existing code, but don't see any examples of that.  So,
> your naming is probably as good as any.
> 
> > +++ b/src/qemu/qemu_capabilities.c
> > @@ -687,6 +687,7 @@ virQEMUCapsInitGuest(virCapsPtr caps,
> >      virQEMUCapsPtr qemubinCaps = NULL;
> >      virQEMUCapsPtr kvmbinCaps = NULL;
> >      int ret = -1;
> > +    bool hasdisksnapshot = false;
> >  
> >      /* Check for existence of base emulator, or alternate base
> >       * which can be used with magic cpu choice
> > @@ -774,6 +775,12 @@ virQEMUCapsInitGuest(virCapsPtr caps,
> >          !virCapabilitiesAddGuestFeature(guest, "deviceboot", 1, 0))
> 
> That, and you were copying from 'deviceboot' which also uses squashed style.

Yes. It looks to me the squashed style was slightly more common, so I adopted it.

> 
> >          goto error;
> >  
> > +    if (virQEMUCapsGet(qemubinCaps, QEMU_CAPS_DISK_SNAPSHOT))
> > +        hasdisksnapshot = true;
> > +
> > +    if (!virCapabilitiesAddGuestFeature(guest, "disksnapshot",
> > hasdisksnapshot, 0))
> > +        goto error;
> 
> I probably would have skipped the temporary variable, but then hit
> longer line length: (snip)

Yep. That's the reason I used the temporary.

> Preliminary ACK, assuming the rest of the series covers testing and
> documentation (and as you said in the cover letter, existing docs are a
> bit sparse).

Thanks. Will send an amended second version (so far for the typos
in the commit message and for the capabilityschemadata tests)
once all the patches in the series are reviewed.

Thanks and best regards,

-- 
Francesco Romani
RedHat Engineering Virtualization R & D
Phone: 8261328
IRC: fromani