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(a)redhat.com https://www.redhat.com/mailman/listinfo/libvir-list