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

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



On 30.03.2012 09:50, 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);

ACK

Maybe it's worth squashing this in:

diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 3d2bb6b..3a14727 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -3444,8 +3444,7 @@ qemuBuildVirtioSerialPortDevStr(virDomainChrDefPtr
dev,

     if (dev->deviceType == VIR_DOMAIN_CHR_DEVICE_TYPE_CHANNEL &&
         dev->source.type == VIR_DOMAIN_CHR_TYPE_SPICEVMC &&
-        dev->target.name &&
-        STRNEQ(dev->target.name, "com.redhat.spice.0")) {
+        STRNEQ_NULLABLE(dev->target.name, "com.redhat.spice.0")) {
         qemuReportError(VIR_ERR_CONFIG_UNSUPPORTED,
                         _("Unsupported spicevmc target name '%s'"),
                         dev->target.name);


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