[libvirt] [PATCH] qemu: Don't parse device twice in attach/detach

[Eric Blake]

On 03/01/2012 11:56 AM, Michal Privoznik wrote:
> Some nits are generated during XML parse (e.g. MAC address of

This reads awkwardly; how about:

s/nits/members/

> an interface); However, with current implementation, if we
> are plugging a device both to persistent and live config,
> we parse given XML twice: first time for live, second for config.
> This is wrong then as the second time we are not guaranteed
> to generate same values as we did for the first time.
> To prevent that we need to create a copy of DeviceDefPtr;
> This is done through format/parse process instead of writing
> functions for deep copy as it is easier to maintain:
> adding new field to any virDomain*DefPtr doesn't require change
> of copying function.
> ---
>  src/conf/domain_conf.c   |   94 ++++++++++++++++++++++++++++++++++++++++++++++
>  src/conf/domain_conf.h   |    3 +
>  src/libvirt_private.syms |    1 +
>  src/qemu/qemu_driver.c   |   40 +++++++++++--------
>  4 files changed, 121 insertions(+), 17 deletions(-)
> 

> +
> +#define VIR_DOMAIN_DEVICE_FORMAT(func) \
> +    if (func < 0) \
> +        goto cleanup; \
> +    xmlStr = virBufferContentAndReset(&buf); \
> +    ret = virDomainDeviceDefParse(caps, def, xmlStr, flags)

If you sink these two lines to occur after the switch statement closes,
then you can avoid this macro.  That is...

> +
> +virDomainDeviceDefPtr
> +virDomainDeviceDefCopy(virCapsPtr caps,
> +                       const virDomainDefPtr def,
> +                       virDomainDeviceDefPtr src)
> +{
> +    virDomainDeviceDefPtr ret = NULL;
> +    virBuffer buf = VIR_BUFFER_INITIALIZER;
> +    int flags = VIR_DOMAIN_XML_INACTIVE;
> +    char *xmlStr = NULL;

int rc;

> +
> +    switch(src->type) {
> +    case VIR_DOMAIN_DEVICE_DISK:
> +        VIR_DOMAIN_DEVICE_FORMAT(virDomainDiskDefFormat(&buf,
> +                                                        src->data.disk,
> +                                                        flags));

case VIR_DOMAIN_DEVICE_DISK:
    rc = virDomainDiskDefFormat(&buf, src->data.disk, flags);
    break;

...
> +    default:
> +        virDomainReportError(VIR_ERR_INTERNAL_ERROR,
> +                             _("Copying definition of '%d' type "
> +                               "is not implemented yet."),
> +                             src->type);

goto cleanup;

> +        break;
> +    }

if (fc < 0)
    goto cleanup;
xmlStr = virBufferContentAndReset(&buf);
ret = virDomainDeviceDefParse(caps, def, xmlStr, flags);

> +
> +cleanup:
> +    VIR_FREE(xmlStr);
> +    return ret;
> +}

> @@ -5694,6 +5698,8 @@ endjob:
>  cleanup:
>      virDomainDefFree(vmdef);
>      virDomainDeviceDefFree(dev);
> +    if (free_dev_copy)
> +        virDomainDeviceDefFree(dev_copy);

You could also avoid free_dev_copy, and just say if (dev != dev_copy) here.

Overall, this looks like a reasonable patch.  But I suggested enough
cleanups that it's probably worth seeing a v2.

-- 
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/20120302/7e429cc2/attachment-0001.sig>