[libvirt] [RFC: PATCHv3 3/3] save: generate idempotent inactive xml for running domain

[Laine Stump]

On 07/22/2011 12:21 AM, Eric Blake wrote:
> Noticed by comparing the xml generated by virDomainSave with the
> xml produced by reparsing and redumping that xml.
>
> * src/conf/domain_conf.c (virDomainDeviceInfoIsSet): Add
> parameter, and update all callers.  Make static.
> (virDomainNetDefFormat): Skip generated ifname.
> * src/conf/domain_conf.h (virDomainDeviceInfoIsSet): Delete.
> * src/libvirt_private.syms (domain_conf.h): Update.
> ---
>
> Sending this now, to get review started, but I still have some
> more fixing to do - right now, active domains still include:
>
> +<seclabel type='dynamic' model='selinux' relabel='yes'/>
>
> which is not present on reparse, but I'm too tired to find out why.


I know the feeling :-)


So does it turn out that this is important, or not?


>   src/conf/domain_conf.c   |   26 ++++++++++++++++----------
>   src/conf/domain_conf.h   |    1 -
>   src/libvirt_private.syms |    1 -
>   3 files changed, 16 insertions(+), 12 deletions(-)
>
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 919a75a..52e8ada 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -1388,11 +1388,12 @@ int virDomainDeviceVirtioSerialAddressIsValid(
>   }
>
>
> -int virDomainDeviceInfoIsSet(virDomainDeviceInfoPtr info)
> +static int
> +virDomainDeviceInfoIsSet(virDomainDeviceInfoPtr info, unsigned int flags)
>   {
>       if (info->type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE)
>           return 1;
> -    if (info->alias)
> +    if (info->alias&&  !(flags&  VIR_DOMAIN_XML_INACTIVE))
>           return 1;
>       return 0;
>   }
> @@ -8580,7 +8581,7 @@ virDomainControllerDefFormat(virBufferPtr buf,
>           break;
>       }
>
> -    if (virDomainDeviceInfoIsSet(&def->info)) {
> +    if (virDomainDeviceInfoIsSet(&def->info, flags)) {
>           virBufferAddLit(buf, ">\n");
>           if (virDomainDeviceInfoFormat(buf,&def->info, flags)<  0)
>               return -1;
> @@ -8806,9 +8807,14 @@ virDomainNetDefFormat(virBufferPtr buf,
>           break;
>       }
>
> -    if (def->ifname)
> +
> +    if (def->ifname&&
> +        !((flags&  VIR_DOMAIN_XML_INACTIVE)&&
> +          (STRPREFIX(def->ifname, "vnet")))) {
> +        /* Skip auto-generated target names for inactive config. */


It's kind of bothersome that use of this magic device name prefix isn't 
self-contained in domain_conf.c (or somewhere else). Perhaps the string 
could be defined in domain_conf.h, then used here and in qemu_command.c 
(is it used any place else?).

Aside from that bit of ugliness, ACK. (And I could live with this. at 
least temporarily, as well, although making all the places work off a 
single string constant might hypothetically save someone lots of 
frustration in some uncharted future.)

>           virBufferEscapeString(buf, "<target dev='%s'/>\n",
>                                 def->ifname);
> +    }
>       if (def->model) {
>           virBufferEscapeString(buf, "<model type='%s'/>\n",
>                                 def->model);
> @@ -9041,7 +9047,7 @@ virDomainChrDefFormat(virBufferPtr buf,
>           break;
>       }
>
> -    if (virDomainDeviceInfoIsSet(&def->info)) {
> +    if (virDomainDeviceInfoIsSet(&def->info, flags)) {
>           if (virDomainDeviceInfoFormat(buf,&def->info, flags)<  0)
>               return -1;
>       }
> @@ -9069,7 +9075,7 @@ virDomainSmartcardDefFormat(virBufferPtr buf,
>       virBufferAsprintf(buf, "<smartcard mode='%s'", mode);
>       switch (def->type) {
>       case VIR_DOMAIN_SMARTCARD_TYPE_HOST:
> -        if (!virDomainDeviceInfoIsSet(&def->info)) {
> +        if (!virDomainDeviceInfoIsSet(&def->info, flags)) {
>               virBufferAddLit(buf, "/>\n");
>               return 0;
>           }
> @@ -9119,7 +9125,7 @@ virDomainSoundDefFormat(virBufferPtr buf,
>       virBufferAsprintf(buf, "<sound model='%s'",
>                         model);
>
> -    if (virDomainDeviceInfoIsSet(&def->info)) {
> +    if (virDomainDeviceInfoIsSet(&def->info, flags)) {
>           virBufferAddLit(buf, ">\n");
>           if (virDomainDeviceInfoFormat(buf,&def->info, flags)<  0)
>               return -1;
> @@ -9148,7 +9154,7 @@ virDomainMemballoonDefFormat(virBufferPtr buf,
>       virBufferAsprintf(buf, "<memballoon model='%s'",
>                         model);
>
> -    if (virDomainDeviceInfoIsSet(&def->info)) {
> +    if (virDomainDeviceInfoIsSet(&def->info, flags)) {
>           virBufferAddLit(buf, ">\n");
>           if (virDomainDeviceInfoFormat(buf,&def->info, flags)<  0)
>               return -1;
> @@ -9197,7 +9203,7 @@ virDomainWatchdogDefFormat(virBufferPtr buf,
>       virBufferAsprintf(buf, "<watchdog model='%s' action='%s'",
>                         model, action);
>
> -    if (virDomainDeviceInfoIsSet(&def->info)) {
> +    if (virDomainDeviceInfoIsSet(&def->info, flags)) {
>           virBufferAddLit(buf, ">\n");
>           if (virDomainDeviceInfoFormat(buf,&def->info, flags)<  0)
>               return -1;
> @@ -9280,7 +9286,7 @@ virDomainInputDefFormat(virBufferPtr buf,
>       virBufferAsprintf(buf, "<input type='%s' bus='%s'",
>                         type, bus);
>
> -    if (virDomainDeviceInfoIsSet(&def->info)) {
> +    if (virDomainDeviceInfoIsSet(&def->info, flags)) {
>           virBufferAddLit(buf, ">\n");
>           if (virDomainDeviceInfoFormat(buf,&def->info, flags)<  0)
>               return -1;
> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
> index 551946b..b97a57a 100644
> --- a/src/conf/domain_conf.h
> +++ b/src/conf/domain_conf.h
> @@ -1388,7 +1388,6 @@ int virDomainDeviceAddressIsValid(virDomainDeviceInfoPtr info,
>   int virDomainDevicePCIAddressIsValid(virDomainDevicePCIAddressPtr addr);
>   int virDomainDeviceDriveAddressIsValid(virDomainDeviceDriveAddressPtr addr);
>   int virDomainDeviceVirtioSerialAddressIsValid(virDomainDeviceVirtioSerialAddressPtr addr);
> -int virDomainDeviceInfoIsSet(virDomainDeviceInfoPtr info);
>   void virDomainDeviceInfoClear(virDomainDeviceInfoPtr info);
>   void virDomainDefClearPCIAddresses(virDomainDefPtr def);
>   void virDomainDefClearDeviceAliases(virDomainDefPtr def);
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index e7d2579..3996efa 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -267,7 +267,6 @@ virDomainDeviceAddressIsValid;
>   virDomainDeviceAddressTypeToString;
>   virDomainDeviceDefFree;
>   virDomainDeviceDefParse;
> -virDomainDeviceInfoIsSet;
>   virDomainDeviceInfoIterate;
>   virDomainDevicePCIAddressIsValid;
>   virDomainDeviceTypeToString;