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

[Laine Stump <laine laine org>]

On 03/30/2012 11:06 AM, Michal Privoznik wrote:
> 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")) {

Sure, that's a nice reduction in line count, and optimization.

I pushed it with that change.

Thanks! (to Eric and Christophe as well).