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