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).