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@xxxxxxxxxx > https://www.redhat.com/mailman/listinfo/libvir-list -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list