[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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/

> is available.
> 
> Without this information available, the only way to check
> for the snapshot support is to request one and check for
> errors.
> 
> Signed-off-by: Francesco Romani <fromani redhat com>
> ---
>  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:

if (!virCapabilitiesAddGuestFeature(guest, "disksnapshot",
                                    virQEMUCapsGet(qemubinCaps,

QEMU_CAPS_DISK_SNAPSHOT)),
                                    0))
    goto error;

so your way is fine as-is.

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

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

Attachment: signature.asc
Description: OpenPGP digital signature


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]