[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]