Re: [libvirt] [PATCH] qemu: set default name for SPICE agent channel when generating command
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@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
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")) { qemuReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("Unsupported spicevmc target name '%s'"), dev->target.name); -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu: set default name for SPICE agent channel when generating command
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 pgpimzUk9IT7d.pgp Description: PGP signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] qemu: set default name for SPICE agent channel when generating command
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