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

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



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