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).
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list