[libvirt] [PATCH] Set default name for SPICE agent channel
libvirt documentation for channels with type 'spicevmc' says that the 'target' child node has: "an optional attribute name controls how the guest will have access to the channel, and defaults to name='com.redhat.spice.0'." However, this default value is never set in libvirt code base, there's only a check in qemu_command.c to error out if the name attribute doesn't have the expected value (if it's set). This commit sets a default target name for spicevmc channels during the domain configuration parsing so that the code agrees with the documentation. --- src/conf/domain_conf.c | 7 +++++++ 1 files changed, 7 insertions(+), 0 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index d5def1c..9012462 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -5218,6 +5218,13 @@ 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; + } + } } } -- 1.7.7.6
On 03/23/2012 04:08 AM, Christophe Fergeau wrote:
libvirt documentation for channels with type 'spicevmc' says that the 'target' child node has: "an optional attribute name controls how the guest will have access to the channel, and defaults to name='com.redhat.spice.0'."
However, this default value is never set in libvirt code base, there's only a check in qemu_command.c to error out if the name attribute doesn't have the expected value (if it's set).
This commit sets a default target name for spicevmc channels during the domain configuration parsing so that the code agrees with the documentation. --- src/conf/domain_conf.c | 7 +++++++ 1 files changed, 7 insertions(+), 0 deletions(-)
ACK, and okay to push before 0.9.11.
} 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; + } + } } }
-- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On Wed, Mar 28, 2012 at 10:02:40AM -0600, Eric Blake wrote:
On 03/23/2012 04:08 AM, Christophe Fergeau wrote:
libvirt documentation for channels with type 'spicevmc' says that the 'target' child node has: "an optional attribute name controls how the guest will have access to the channel, and defaults to name='com.redhat.spice.0'."
However, this default value is never set in libvirt code base, there's only a check in qemu_command.c to error out if the name attribute doesn't have the expected value (if it's set).
This commit sets a default target name for spicevmc channels during the domain configuration parsing so that the code agrees with the documentation. --- src/conf/domain_conf.c | 7 +++++++ 1 files changed, 7 insertions(+), 0 deletions(-)
ACK, and okay to push before 0.9.11.
Thanks, pushed. Christophe
On 03/28/2012 12:02 PM, Eric Blake wrote:
On 03/23/2012 04:08 AM, Christophe Fergeau wrote:
libvirt documentation for channels with type 'spicevmc' says that the 'target' child node has: "an optional attribute name controls how the guest will have access to the channel, and defaults to name='com.redhat.spice.0'."
However, this default value is never set in libvirt code base, there's only a check in qemu_command.c to error out if the name attribute doesn't have the expected value (if it's set).
This commit sets a default target name for spicevmc channels during the domain configuration parsing so that the code agrees with the documentation. --- src/conf/domain_conf.c | 7 +++++++ 1 files changed, 7 insertions(+), 0 deletions(-) ACK, and okay to push before 0.9.11.
} 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; + } + } } }
I know I'm a bit late to the party (I missed your mail before), but would it have been possible to do the "default" processing in the caller rather than in the parsing function itself? Setting it here means that if the default changes, previously defined guests will be stuck with the old default (since it will be encoded into re-formatted XML saved to disk), and assigning default values in parsing functions has in the past led to trouble (e.g. the fact that virDomainNetDefParseXML adds a mac address when one isn't specified caused trouble because parsing the same XML twice gave difference results; that particular problem wouldn't happen here, obviously).
On Wed, Mar 28, 2012 at 03:20:45PM -0400, Laine Stump wrote:
I know I'm a bit late to the party (I missed your mail before), but would it have been possible to do the "default" processing in the caller rather than in the parsing function itself?
I cannot disagree since I was not sure at all that this was the right place for setting defaults, I should have mentioned this explicitly when sending the patch.. Who is "the caller" exactly in this case? Thanks, Christophe
On 03/29/2012 05:52 AM, Christophe Fergeau wrote:
I know I'm a bit late to the party (I missed your mail before), but would it have been possible to do the "default" processing in the caller rather than in the parsing function itself? I cannot disagree since I was not sure at all that this was the right place for setting defaults, I should have mentioned this explicitly when sending
On Wed, Mar 28, 2012 at 03:20:45PM -0400, Laine Stump wrote: the patch.. Who is "the caller" exactly in this case?
I'm not previously familiar with this attribute, but it looks like the only place it's used is in qemuBuildVirtioSerialPortDevStr, where it does the following: 1) if it's set, and doesn't == "com.redhat.spice.0" it logs an error and fails 2) if it's set, the option ",name=com.redhat.spice.0" is added to the device string. So it looks like you could get the same effect by changing that last bit to either add the contents of dev->target.name, or if that's NULL, "com.redhat.spice.0". In this particular case I don't know if it's going to make any difference in practice; I'm just always wary of parser code that modifies the output such that it doesn't mirror the input. (One way of thinking about it is that if you were to set everything in an object, then do a Format/Parse roundtrip, the results would be different. That's going to end up being the case anyway because there's already so much existing parse code that commits this sin (my opinion), but I think it's good to reduce the amount of this type behavior as much as possible. Does anyone with more experience with the libvirt spice code have any opinion on whether it makes any difference if we add in the default in the parsing function or just when we build the commandline? danpb?
On 03/29/2012 03:10 PM, Laine Stump wrote:
2) if it's set, the option ",name=com.redhat.spice.0" is added to the device string.
So it looks like you could get the same effect by changing that last bit to either add the contents of dev->target.name, or if that's NULL, "com.redhat.spice.0".
I'd be fine with a patch along these lines that moves the default out of domain_conf.c and into the command line generator.
Does anyone with more experience with the libvirt spice code have any opinion on whether it makes any difference if we add in the default in the parsing function or just when we build the commandline? danpb?
Delaying the default until command line generation should be just fine. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
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); -- 1.7.7.6
Hey, Patch looks good to me, thanks for writing it :) Christophe On Fri, Mar 30, 2012 at 03:50:37AM -0400, 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); -- 1.7.7.6
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
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);
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).
participants (4)
-
Christophe Fergeau -
Eric Blake -
Laine Stump -
Michal Privoznik