[libvirt] [PATCH] qemu: Don't parse device twice in attach/detach

[Michal Privoznik]

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