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

From: "Eric Blake" <eblake(a)redhat.com&gt; To: "Francesco Romani" <fromani(a)redhat.com&gt;, libvir-list(a)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/

> 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...

> </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.

> 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)

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).