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")) {