[libvirt] [PATCH] qemu: set default name for SPICE agent channel when generating command

[Christophe Fergeau]

Hey,

Patch looks good to me, thanks for writing it :)

Christophe

On Fri, Mar 30, 2012 at 03:50:37AM -0400, Laine Stump wrote:
> commit b0e2bb33 set a default value for the SPICE agent channel by
> inserting it during parsing of the channel XML. That method of setting
> a default is problematic because it makes a format/parse roundtrip
> unclean, and experience with setting other values as a side effect of
> parsing has led to headaches (e.g. automatically setting a MAC address
> in the parser when one isn't specified in the input XML).
> 
> This patch reverts commit b0e2bb33 and replaces it with the alternate
> implementation of simply inserting the default value in the
> appropriate place on the qemu commandline when no value is provided.
> ---
> 
> (Playing the devil's advocate on my own patch for a moment - one
> advantage of Christophe's method of setting the default is that if we
> use that object somewhere else in libvirt's code in the future, the
> value will be set even if the XML left it unset, but with my method we
> will have to check for a NULL name and replace it with the default
> value anywhere we want to use it. So I won't say that either method is
> definitely the proper way to go, but will just present this
> alternative and see if someone else has an even stronger opinion than
> me :-)
> 
> (BTW, I think I've decided while typing this message that, if we
> decide to change from the original method of setting default to this
> new method, the change should be pushed as two separate patches - one
> reverting the original, and another adding the new. It's too close to
> morning for me to take the time to do that right now, though.)
> 
>  src/conf/domain_conf.c  |    7 -------
>  src/qemu/qemu_command.c |    6 +++---
>  2 files changed, 3 insertions(+), 10 deletions(-)
> 
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 24e10e6..ea558bb 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -5248,13 +5248,6 @@ virDomainChrDefParseXML(virCapsPtr caps,
>              goto error;
>          } else {
>              def->source.data.spicevmc = VIR_DOMAIN_CHR_SPICEVMC_VDAGENT;
> -            if (!def->target.name) {
> -                def->target.name = strdup("com.redhat.spice.0");
> -                if (!def->target.name) {
> -                    virReportOOMError();
> -                    goto error;
> -                }
> -            }
>          }
>      }
>  
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index 3d2bb6b..50b1e6d 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -3457,9 +3457,9 @@ qemuBuildVirtioSerialPortDevStr(virDomainChrDefPtr dev,
>            qemuCapsGet(qemuCaps, QEMU_CAPS_DEVICE_SPICEVMC))) {
>          virBufferAsprintf(&buf, ",chardev=char%s,id=%s",
>                            dev->info.alias, dev->info.alias);
> -        if (dev->deviceType == VIR_DOMAIN_CHR_DEVICE_TYPE_CHANNEL &&
> -            dev->target.name) {
> -            virBufferAsprintf(&buf, ",name=%s", dev->target.name);
> +        if (dev->deviceType == VIR_DOMAIN_CHR_DEVICE_TYPE_CHANNEL) {
> +            virBufferAsprintf(&buf, ",name=%s", dev->target.name
> +                              ? dev->target.name : "com.redhat.spice.0");
>          }
>      } else {
>          virBufferAsprintf(&buf, ",id=%s", dev->info.alias);
> -- 
> 1.7.7.6
> 
> --
> libvir-list mailing list
> libvir-list at redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20120330/903d4bfa/attachment-0001.sig>