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@xxxxxxxxxx +1-919-301-3266
Libvirt virtualization library http://libvirt.org
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list