Re: [libvirt] [PATCH] qemu: Don't parse device twice in attach/detach
On 07.03.2012 19:46, Laine Stump wrote: > On 03/07/2012 12:48 PM, Michal Privoznik wrote: >> Some nits are generated during XML parse (e.g. MAC address of >> 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. >> --- >> diff to v1: >> -Eric's review included >> >> src/conf/domain_conf.c | 70 >> ++ >> src/conf/domain_conf.h |3 ++ >> src/libvirt_private.syms |1 + >> src/qemu/qemu_driver.c | 38 ++--- >> 4 files changed, 95 insertions(+), 17 deletions(-) >> >> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c >> index b994718..42cfbd5 100644 >> --- a/src/conf/domain_conf.c >> +++ b/src/conf/domain_conf.c >> @@ -14529,3 +14529,73 @@ virDomainNetFind(virDomainDefPtr def, const char >> *device) >> >> return net; >> } > > > I still think there should be a comment added here saying something like: > > NB: virDomainDeviceDefCopy does a deep copy of only the parts of a > DeviceDef that are valid when just the flag VIR_DOMAIN_XML_INACTIVE is > set. This means that any part of the device xml that is conditionally > parsed/formatted based on some other flag being set (or on the INACTIVE > flag being reset) *will not* be copied to the destination. Caveat emptor. > >> + >> +virDomainDeviceDefPtr >> +virDomainDeviceDefCopy(virCapsPtr caps, >> + const virDomainDefPtr def, >> + virDomainDeviceDefPtr src) > > > Otherwise it looks like you've taken care of all of Eric's issues, and > seems clean, so ACK from me (conditional on adding a comment documenting > the limitations of the new copy function). Thank you both; Added comment and pushed. Michal > > -- > libvir-list mailing list > libvir-list@redhat.com > https://www.redhat.com/mailman/listinfo/libvir-list -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu: Don't parse device twice in attach/detach
On 03/07/2012 12:48 PM, Michal Privoznik wrote: > Some nits are generated during XML parse (e.g. MAC address of > 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. > --- > diff to v1: > -Eric's review included > > src/conf/domain_conf.c | 70 > ++ > src/conf/domain_conf.h |3 ++ > src/libvirt_private.syms |1 + > src/qemu/qemu_driver.c | 38 ++--- > 4 files changed, 95 insertions(+), 17 deletions(-) > > diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c > index b994718..42cfbd5 100644 > --- a/src/conf/domain_conf.c > +++ b/src/conf/domain_conf.c > @@ -14529,3 +14529,73 @@ virDomainNetFind(virDomainDefPtr def, const char > *device) > > return net; > } I still think there should be a comment added here saying something like: NB: virDomainDeviceDefCopy does a deep copy of only the parts of a DeviceDef that are valid when just the flag VIR_DOMAIN_XML_INACTIVE is set. This means that any part of the device xml that is conditionally parsed/formatted based on some other flag being set (or on the INACTIVE flag being reset) *will not* be copied to the destination. Caveat emptor. > + > +virDomainDeviceDefPtr > +virDomainDeviceDefCopy(virCapsPtr caps, > + const virDomainDefPtr def, > + virDomainDeviceDefPtr src) Otherwise it looks like you've taken care of all of Eric's issues, and seems clean, so ACK from me (conditional on adding a comment documenting the limitations of the new copy function). -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu: Don't parse device twice in attach/detach
On 03/07/2012 10:48 AM, Michal Privoznik wrote: > Some nits are generated during XML parse (e.g. MAC address of 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. > --- > diff to v1: > -Eric's review included except for in the commit message itself :) ACK. But Laine had comments on v1 as well, in case you want to wait for a second opinion on whether this needs any more work. -- Eric Blake ebl...@redhat.com+1-919-301-3266 Libvirt virtualization library http://libvirt.org signature.asc Description: OpenPGP digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] qemu: Don't parse device twice in attach/detach
Some nits are generated during XML parse (e.g. MAC address of
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.
---
diff to v1:
-Eric's review included
src/conf/domain_conf.c | 70 ++
src/conf/domain_conf.h |3 ++
src/libvirt_private.syms |1 +
src/qemu/qemu_driver.c | 38 ++---
4 files changed, 95 insertions(+), 17 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index b994718..42cfbd5 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -14529,3 +14529,73 @@ virDomainNetFind(virDomainDefPtr def, const char
*device)
return net;
}
+
+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 = -1;
+
+switch(src->type) {
+case VIR_DOMAIN_DEVICE_DISK:
+rc = virDomainDiskDefFormat(&buf, src->data.disk, flags);
+break;
+case VIR_DOMAIN_DEVICE_LEASE:
+rc = virDomainLeaseDefFormat(&buf, src->data.lease);
+break;
+case VIR_DOMAIN_DEVICE_FS:
+rc = virDomainFSDefFormat(&buf, src->data.fs, flags);
+break;
+case VIR_DOMAIN_DEVICE_NET:
+rc = virDomainNetDefFormat(&buf, src->data.net, flags);
+break;
+case VIR_DOMAIN_DEVICE_INPUT:
+rc = virDomainInputDefFormat(&buf, src->data.input, flags);
+break;
+case VIR_DOMAIN_DEVICE_SOUND:
+rc = virDomainSoundDefFormat(&buf, src->data.sound, flags);
+break;
+case VIR_DOMAIN_DEVICE_VIDEO:
+rc = virDomainVideoDefFormat(&buf, src->data.video, flags);
+break;
+case VIR_DOMAIN_DEVICE_HOSTDEV:
+rc = virDomainHostdevDefFormat(&buf, src->data.hostdev, flags);
+break;
+case VIR_DOMAIN_DEVICE_WATCHDOG:
+rc = virDomainWatchdogDefFormat(&buf, src->data.watchdog, flags);
+break;
+case VIR_DOMAIN_DEVICE_CONTROLLER:
+rc = virDomainControllerDefFormat(&buf, src->data.controller, flags);
+break;
+case VIR_DOMAIN_DEVICE_GRAPHICS:
+rc = virDomainGraphicsDefFormat(&buf, src->data.graphics, flags);
+break;
+case VIR_DOMAIN_DEVICE_HUB:
+rc = virDomainHubDefFormat(&buf, src->data.hub, flags);
+break;
+case VIR_DOMAIN_DEVICE_REDIRDEV:
+rc = virDomainRedirdevDefFormat(&buf, src->data.redirdev, flags);
+break;
+default:
+virDomainReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Copying definition of '%d' type "
+ "is not implemented yet."),
+ src->type);
+goto cleanup;
+}
+
+if (rc < 0)
+goto cleanup;
+
+xmlStr = virBufferContentAndReset(&buf);
+ret = virDomainDeviceDefParse(caps, def, xmlStr, flags);
+
+cleanup:
+VIR_FREE(xmlStr);
+return ret;
+}
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index 87b4103..1453822 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -1794,6 +1794,9 @@ void virDomainHostdevDefFree(virDomainHostdevDefPtr def);
void virDomainHubDefFree(virDomainHubDefPtr def);
void virDomainRedirdevDefFree(virDomainRedirdevDefPtr def);
void virDomainDeviceDefFree(virDomainDeviceDefPtr def);
+virDomainDeviceDefPtr virDomainDeviceDefCopy(virCapsPtr caps,
+ const virDomainDefPtr def,
+ virDomainDeviceDefPtr src);
int virDomainDeviceAddressIsValid(virDomainDeviceInfoPtr info,
int type);
int virDomainDevicePCIAddressIsValid(virDomainDevicePCIAddressPtr addr);
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index c44c617..a4cf199 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -287,6 +287,7 @@ virDomainDeviceAddressIsValid;
virDomainDeviceAddressPciMultiTypeFromString;
virDomainDeviceAddressPciMultiTypeToString;
virDomainDeviceAddressTypeToString;
+virDomainDeviceDefCopy;
virDomainDeviceDefFree;
virDomainDeviceDefParse;
virDomainDeviceInfoIterate;
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index ddb381a..1d121cf 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -5563,7 +5563,7 @@
Re: [libvirt] [PATCH] qemu: Don't parse device twice in attach/detach
On 03/01/2012 01:56 PM, Michal Privoznik wrote:
> Some nits are generated during XML parse (e.g. MAC address of
> 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(-)
>
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index f9654f1..f001319 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -14118,3 +14118,97 @@ virDomainNetFind(virDomainDefPtr def, const char
> *device)
>
> return net;
> }
> +
> +#define VIR_DOMAIN_DEVICE_FORMAT(func) \
> +if (func < 0) \
> +goto cleanup; \
> +xmlStr = virBufferContentAndReset(&buf); \
> +ret = virDomainDeviceDefParse(caps, def, xmlStr, flags)
> +
> +virDomainDeviceDefPtr
> +virDomainDeviceDefCopy(virCapsPtr caps,
> + const virDomainDefPtr def,
> + virDomainDeviceDefPtr src)
> +{
> +virDomainDeviceDefPtr ret = NULL;
> +virBuffer buf = VIR_BUFFER_INITIALIZER;
> +int flags = VIR_DOMAIN_XML_INACTIVE;
I'm concerned that this function has the appearance of being general
purpose, but actually isn't. There are many bits of data in
virDomainNetDef that won't be copied, because they're only
parsed/formatted if extra flags are given in the calls to Parse and
Format (VIR_DOMAIN_XML_INTERNAL_STATUS,
VIR_DOMAIN_XML_INTERNAL_STATUS_ACTUAL_NET, etc).
For the current use case, it should work adequately, since we know that
we just created the object by parsing with a limited flag set, but I can
imagine the day 6 months from now when somebody who doesn't know the
history/limitations of ths new function finds it and mistakenly believes
it's the answer to their current (probably very different) problem (and
then commits code with a bug that doesn't show up until it's in the field)
It's probably too much trouble in the short term to try and make this
function completely general purpose, but I think at least the function
name should reflect the limited nature and/or both the declaration and
definition of the function should be accompanied by a comment noting the
limitations.
Beyond that, this looks like the right way to fix the problem. Note that
libxl_driver.c:libxmlDomainModifyDeviceFlags() has exactly the same
problem, so maybe its fix can be included in this patch.
> +char *xmlStr = NULL;
> +
> +switch(src->type) {
> +case VIR_DOMAIN_DEVICE_DISK:
> +VIR_DOMAIN_DEVICE_FORMAT(virDomainDiskDefFormat(&buf,
> +src->data.disk,
> +flags));
> +break;
> +case VIR_DOMAIN_DEVICE_LEASE:
> +VIR_DOMAIN_DEVICE_FORMAT(virDomainLeaseDefFormat(&buf,
> + src->data.lease));
> +break;
> +case VIR_DOMAIN_DEVICE_FS:
> +VIR_DOMAIN_DEVICE_FORMAT(virDomainFSDefFormat(&buf,
> + src->data.fs,
> + flags));
> +break;
> +case VIR_DOMAIN_DEVICE_NET:
> +VIR_DOMAIN_DEVICE_FORMAT(virDomainNetDefFormat(&buf,
> + src->data.net,
> + flags));
> +break;
> +case VIR_DOMAIN_DEVICE_INPUT:
> +VIR_DOMAIN_DEVICE_FORMAT(virDomainInputDefFormat(&buf,
> + src->data.input,
> + flags));
> +break;
> +case VIR_DOMAIN_DEVICE_SOUND:
> +VIR_DOMAIN_DEVICE_FORMAT(virDomainSoundDefFormat(&buf,
> + src->data.sound,
> + flags));
> +break;
> +case VIR_DOMAIN_DEVICE_VIDEO:
> +VIR_DOMAIN_DEVICE_FORMAT(virDomainVideoDefFormat(&buf,
> + src->data.video,
> + flags));
> +break;
> +case VIR_DOMAIN_DEVICE_HOSTDEV:
> +VIR_DOMAIN_DEVICE_FORMAT(virDomainHostdevDefFormat(&buf,
> +
Re: [libvirt] [PATCH] qemu: Don't parse device twice in attach/detach
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 ebl...@redhat.com+1-919-301-3266
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] qemu: Don't parse device twice in attach/detach
Some nits are generated during XML parse (e.g. MAC address of
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(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index f9654f1..f001319 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -14118,3 +14118,97 @@ virDomainNetFind(virDomainDefPtr def, const char
*device)
return net;
}
+
+#define VIR_DOMAIN_DEVICE_FORMAT(func) \
+if (func < 0) \
+goto cleanup; \
+xmlStr = virBufferContentAndReset(&buf); \
+ret = virDomainDeviceDefParse(caps, def, xmlStr, flags)
+
+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;
+
+switch(src->type) {
+case VIR_DOMAIN_DEVICE_DISK:
+VIR_DOMAIN_DEVICE_FORMAT(virDomainDiskDefFormat(&buf,
+src->data.disk,
+flags));
+break;
+case VIR_DOMAIN_DEVICE_LEASE:
+VIR_DOMAIN_DEVICE_FORMAT(virDomainLeaseDefFormat(&buf,
+ src->data.lease));
+break;
+case VIR_DOMAIN_DEVICE_FS:
+VIR_DOMAIN_DEVICE_FORMAT(virDomainFSDefFormat(&buf,
+ src->data.fs,
+ flags));
+break;
+case VIR_DOMAIN_DEVICE_NET:
+VIR_DOMAIN_DEVICE_FORMAT(virDomainNetDefFormat(&buf,
+ src->data.net,
+ flags));
+break;
+case VIR_DOMAIN_DEVICE_INPUT:
+VIR_DOMAIN_DEVICE_FORMAT(virDomainInputDefFormat(&buf,
+ src->data.input,
+ flags));
+break;
+case VIR_DOMAIN_DEVICE_SOUND:
+VIR_DOMAIN_DEVICE_FORMAT(virDomainSoundDefFormat(&buf,
+ src->data.sound,
+ flags));
+break;
+case VIR_DOMAIN_DEVICE_VIDEO:
+VIR_DOMAIN_DEVICE_FORMAT(virDomainVideoDefFormat(&buf,
+ src->data.video,
+ flags));
+break;
+case VIR_DOMAIN_DEVICE_HOSTDEV:
+VIR_DOMAIN_DEVICE_FORMAT(virDomainHostdevDefFormat(&buf,
+ src->data.hostdev,
+ flags));
+break;
+case VIR_DOMAIN_DEVICE_WATCHDOG:
+VIR_DOMAIN_DEVICE_FORMAT(virDomainWatchdogDefFormat(&buf,
+src->data.watchdog,
+flags));
+break;
+case VIR_DOMAIN_DEVICE_CONTROLLER:
+VIR_DOMAIN_DEVICE_FORMAT(virDomainControllerDefFormat(&buf,
+
src->data.controller,
+ flags));
+break;
+case VIR_DOMAIN_DEVICE_GRAPHICS:
+VIR_DOMAIN_DEVICE_FORMAT(virDomainGraphicsDefFormat(&buf,
+src->data.graphics,
+flags));
+break;
+case VIR_DOMAIN_DEVICE_HUB:
+VIR_DOMAIN_DEVICE_FORMAT(virDomainHubDefFormat(&buf,
+ src->data.hub,
+ flags));
+break;
+case VIR_DOMAIN_DEVICE_REDIRDEV:
+VIR_DOMAIN_DEVICE_FORMAT(virDomainRedirdevDefFormat(&buf,
+src->data.redirdev,
+flags));
+break;
+
