[libvirt] [PATCH 3/4] qemu: Implement note API in qemu driver

[Eric Blake]

On 01/13/2012 11:17 AM, Peter Krempa wrote:
> This patch adds support for the new api to change domain descriptions to
> the qemu driver.
> ---
>  src/qemu/qemu_driver.c |   70 ++++++++++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 70 insertions(+), 0 deletions(-)

This seems simple enough that you should support it for more drivers; at
least test and lxc are good candidates.

> +static int
> +qemuDomainSetDescription(virDomainPtr dom, const char *description,
> +                         unsigned int flags)
> +{
> +    struct qemud_driver *driver = dom->conn->privateData;
> +    virDomainObjPtr vm;
> +    virDomainDefPtr persistentDef;
> +    int ret = -1;
> +
> +    virCheckFlags(VIR_DOMAIN_DESCRIPTION_CURRENT |

See my comment in 2/4 about just re-using VIR_DOMAIN_AFFECT_CURRENT
rather than adding a new alias.  Also, this particular enum value is 0,
so you can omit it from this virCheckFlags list.

> +                  VIR_DOMAIN_DESCRIPTION_LIVE |
> +                  VIR_DOMAIN_DESCRIPTION_CONFIG |
> +                  VIR_DOMAIN_DESCRIPTION_NOTE, -1);
> +
> +    bool note = (flags | VIR_DOMAIN_DESCRIPTION_NOTE) > 0;

Wrong operator - you want &, not |.

> +
> +    if (flags | VIR_DOMAIN_DESCRIPTION_LIVE) {

And again.

> +
> +    if (flags | VIR_DOMAIN_DESCRIPTION_CONFIG) {

And again.

> +        if (note) {
> +            VIR_FREE(persistentDef->note);
> +            if (!(persistentDef->note = strdup(description)))
> +                goto oom;
> +        } else {
> +            VIR_FREE(persistentDef->description);
> +            if (!(persistentDef->description = strdup(description)))
> +                goto oom;
> +        }
> +    }

Missing a call to flush the new definition to disk - basically, you need
to call virDomainSaveConfig somewhere in this method.  To test whether
things worked, use virsh to alter the description, then restart
libvirtd, then dumpxml to see if the alterations stuck around.


> +
> +    ret = 0;
> +
> +cleanup:
> +    if (vm)
> +        virDomainObjUnlock(vm);
> +    return ret;
> +oom:

Hmm - elsewhere in qemu_driver.c, we call this sort of label no_memory
or out_of_memory.  Also, I'd consider making it simpler, by not
duplicating the rest of cleanup:

oom:
    virReportOOMError();
    goto cleanup;

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

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 620 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20120113/88338c98/attachment-0001.sig>