Re: [libvirt] [PATCH v2 1/3] qemu: Make all SPICE command-line args optional
On Tue, May 17, 2016 at 08:03:11AM -0400, Cole Robinson wrote: > On 05/17/2016 06:11 AM, Christophe Fergeau wrote: > > Hey, > > > > After this patch series, the QEMU command line may not contain port nor > > tls-port if they both were set to '0'. However, QEMU versions older than > > 2.3.0 will error out because they don't have this commit: > > http://git.qemu.org/?p=qemu.git;a=commitdiff;h=cf7856adefebe86e0 > > > > I assume we want to keep supporting older QEMU binaries, and that this > > needs to be fixed on libvirt side? > > Yes we will want to keep older qemu working. However I think Pavel's patches > address this issue? Actually as Ján pointed out, older QEMU behaviour before/after these patches should be unchanged. Only difference is that before QEMU was always adding port=0/tls-port=0, after the are not present, but QEMU will default to 0 when they are missing. In both cases (present and set to 0, and not present and default to 0), we will be getting an error. I don't think Pavel's patches are going to help there though, but I don't think that's a problem. Christophe signature.asc Description: PGP signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 1/3] qemu: Make all SPICE command-line args optional
On Tue, May 17, 2016 at 02:10:16PM +0200, Ján Tomko wrote: > On Tue, May 17, 2016 at 12:11:38PM +0200, Christophe Fergeau wrote: > > Hey, > > > > After this patch series, the QEMU command line may not contain port nor > > tls-port if they both were set to '0'. However, QEMU versions older than > > 2.3.0 will error out because they don't have this commit: > > http://git.qemu.org/?p=qemu.git;a=commitdiff;h=cf7856adefebe86e0 > > > > From that snippet of code it seems omitting port completely and setting > it to 0 were equivalent. > > > I assume we want to keep supporting older QEMU binaries, and that this > > needs to be fixed on libvirt side? > > It seems this never worked with older QEMUs, regardless of this series. Ah good point, I did not pay enough attention to the commit ;) Christophe signature.asc Description: PGP signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 1/3] qemu: Make all SPICE command-line args optional
On Tue, May 17, 2016 at 12:11:38PM +0200, Christophe Fergeau wrote: > Hey, > > After this patch series, the QEMU command line may not contain port nor > tls-port if they both were set to '0'. However, QEMU versions older than > 2.3.0 will error out because they don't have this commit: > http://git.qemu.org/?p=qemu.git;a=commitdiff;h=cf7856adefebe86e0 > >From that snippet of code it seems omitting port completely and setting it to 0 were equivalent. > I assume we want to keep supporting older QEMU binaries, and that this > needs to be fixed on libvirt side? It seems this never worked with older QEMUs, regardless of this series. Jan -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 1/3] qemu: Make all SPICE command-line args optional
On 05/17/2016 06:11 AM, Christophe Fergeau wrote: > Hey, > > After this patch series, the QEMU command line may not contain port nor > tls-port if they both were set to '0'. However, QEMU versions older than > 2.3.0 will error out because they don't have this commit: > http://git.qemu.org/?p=qemu.git;a=commitdiff;h=cf7856adefebe86e0 > > I assume we want to keep supporting older QEMU binaries, and that this > needs to be fixed on libvirt side? Yes we will want to keep older qemu working. However I think Pavel's patches address this issue? - Cole -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 1/3] qemu: Make all SPICE command-line args optional
Hey,
After this patch series, the QEMU command line may not contain port nor
tls-port if they both were set to '0'. However, QEMU versions older than
2.3.0 will error out because they don't have this commit:
http://git.qemu.org/?p=qemu.git;a=commitdiff;h=cf7856adefebe86e0
I assume we want to keep supporting older QEMU binaries, and that this
needs to be fixed on libvirt side?
Christophe
On Wed, Mar 16, 2016 at 05:45:03PM +0100, Christophe Fergeau wrote:
> The end goal is to avoid adding -spice port=0,addr=127.0.0.1 to QEMU command
> line when no SPICE port is specified in libvirt XML.
>
> Currently, the code relies on port=xx to always be present, so subsequent
> args can be unconditionally appended with a leading ','. Since port=0
> will no longer be added in a subsequent commit, we append a ',' to every
> arg instead of prepending, and remove the last one before adding it to
> the arg list.
> ---
> src/qemu/qemu_command.c | 68
> +++--
> 1 file changed, 38 insertions(+), 30 deletions(-)
>
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index 32d32b1..cd20a16 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -7030,7 +7030,7 @@
> qemuBuildGraphicsSPICECommandLine(virQEMUDriverConfigPtr cfg,
> }
>
> if (port > 0 || tlsPort <= 0)
> -virBufferAsprintf(, "port=%u", port);
> +virBufferAsprintf(, "port=%u,", port);
>
> if (tlsPort > 0) {
> if (!cfg->spiceTLS) {
> @@ -7039,13 +7039,12 @@
> qemuBuildGraphicsSPICECommandLine(virQEMUDriverConfigPtr cfg,
> " but TLS is disabled in qemu.conf"));
> goto error;
> }
> -if (port > 0)
> -virBufferAddChar(, ',');
> -virBufferAsprintf(, "tls-port=%u", tlsPort);
> +
> +virBufferAsprintf(, "tls-port=%u,", tlsPort);
> }
>
> if (cfg->spiceSASL) {
> -virBufferAddLit(, ",sasl");
> +virBufferAddLit(, "sasl,");
>
> if (cfg->spiceSASLdir)
> virCommandAddEnvPair(cmd, "SASL_CONF_PATH",
> @@ -7085,17 +7084,17 @@
> qemuBuildGraphicsSPICECommandLine(virQEMUDriverConfigPtr cfg,
> if (!listenAddr)
> listenAddr = cfg->spiceListen;
> if (listenAddr)
> -virBufferAsprintf(, ",addr=%s", listenAddr);
> +virBufferAsprintf(, "addr=%s,", listenAddr);
>
> VIR_FREE(netAddr);
>
> if (graphics->data.spice.mousemode) {
> switch (graphics->data.spice.mousemode) {
> case VIR_DOMAIN_GRAPHICS_SPICE_MOUSE_MODE_SERVER:
> -virBufferAddLit(, ",agent-mouse=off");
> +virBufferAddLit(, "agent-mouse=off,");
> break;
> case VIR_DOMAIN_GRAPHICS_SPICE_MOUSE_MODE_CLIENT:
> -virBufferAddLit(, ",agent-mouse=on");
> +virBufferAddLit(, "agent-mouse=on,");
> break;
> default:
> break;
> @@ -7106,18 +7105,19 @@
> qemuBuildGraphicsSPICECommandLine(virQEMUDriverConfigPtr cfg,
> * making it visible on CLI, so there's no use of password=XXX
> * in this bit of the code */
> if (!graphics->data.spice.auth.passwd &&
> -!cfg->spicePassword)
> -virBufferAddLit(, ",disable-ticketing");
> +!cfg->spicePassword) {
> +virBufferAddLit(, "disable-ticketing,");
> +}
>
> if (tlsPort > 0)
> -virBufferAsprintf(, ",x509-dir=%s", cfg->spiceTLSx509certdir);
> +virBufferAsprintf(, "x509-dir=%s,", cfg->spiceTLSx509certdir);
>
> switch (defaultMode) {
> case VIR_DOMAIN_GRAPHICS_SPICE_CHANNEL_MODE_SECURE:
> -virBufferAddLit(, ",tls-channel=default");
> +virBufferAddLit(, "tls-channel=default,");
> break;
> case VIR_DOMAIN_GRAPHICS_SPICE_CHANNEL_MODE_INSECURE:
> -virBufferAddLit(, ",plaintext-channel=default");
> +virBufferAddLit(, "plaintext-channel=default,");
> break;
> case VIR_DOMAIN_GRAPHICS_SPICE_CHANNEL_MODE_ANY:
> /* nothing */
> @@ -7133,7 +7133,7 @@
> qemuBuildGraphicsSPICECommandLine(virQEMUDriverConfigPtr cfg,
> "but TLS port is not provided"));
> goto error;
> }
> -virBufferAsprintf(, ",tls-channel=%s",
> +virBufferAsprintf(, "tls-channel=%s,",
>
> virDomainGraphicsSpiceChannelNameTypeToString(i));
> break;
>
> @@ -7144,7 +7144,7 @@
> qemuBuildGraphicsSPICECommandLine(virQEMUDriverConfigPtr cfg,
> "configuration, but plain port is not
> provided"));
> goto error;
> }
> -virBufferAsprintf(, ",plaintext-channel=%s",
> +virBufferAsprintf(, "plaintext-channel=%s,",
>
> virDomainGraphicsSpiceChannelNameTypeToString(i));
> break;
>
> @@ -7175,30 +7175,36 @@
>
Re: [libvirt] [PATCH v2 1/3] qemu: Make all SPICE command-line args optional
On Fri, Mar 18, 2016 at 11:09:25AM +0100, Christophe Fergeau wrote:
> On Fri, Mar 18, 2016 at 10:25:58AM +0100, Ján Tomko wrote:
> > This breaks make syntax-check:
> >
> > Curly brackets around single-line body:
> > src/qemu/qemu_command.c:7559-7561:
> > if (graphics->data.spice.copypaste == VIR_TRISTATE_BOOL_NO) {
> > virBufferAddLit(, "disable-copy-paste,");
> > }
>
> Ah thanks, I'll make sure to fix these before pushing. Totally forgot to
> rerun it before sending v2.
I've now pushed the series after fixing make syntax-check and the
comment.
Christophe
signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v2 1/3] qemu: Make all SPICE command-line args optional
The end goal is to avoid adding -spice port=0,addr=127.0.0.1 to QEMU command
line when no SPICE port is specified in libvirt XML.
Currently, the code relies on port=xx to always be present, so subsequent
args can be unconditionally appended with a leading ','. Since port=0
will no longer be added in a subsequent commit, we append a ',' to every
arg instead of prepending, and remove the last one before adding it to
the arg list.
---
src/qemu/qemu_command.c | 68 +++--
1 file changed, 38 insertions(+), 30 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 32d32b1..cd20a16 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -7030,7 +7030,7 @@ qemuBuildGraphicsSPICECommandLine(virQEMUDriverConfigPtr
cfg,
}
if (port > 0 || tlsPort <= 0)
-virBufferAsprintf(, "port=%u", port);
+virBufferAsprintf(, "port=%u,", port);
if (tlsPort > 0) {
if (!cfg->spiceTLS) {
@@ -7039,13 +7039,12 @@
qemuBuildGraphicsSPICECommandLine(virQEMUDriverConfigPtr cfg,
" but TLS is disabled in qemu.conf"));
goto error;
}
-if (port > 0)
-virBufferAddChar(, ',');
-virBufferAsprintf(, "tls-port=%u", tlsPort);
+
+virBufferAsprintf(, "tls-port=%u,", tlsPort);
}
if (cfg->spiceSASL) {
-virBufferAddLit(, ",sasl");
+virBufferAddLit(, "sasl,");
if (cfg->spiceSASLdir)
virCommandAddEnvPair(cmd, "SASL_CONF_PATH",
@@ -7085,17 +7084,17 @@
qemuBuildGraphicsSPICECommandLine(virQEMUDriverConfigPtr cfg,
if (!listenAddr)
listenAddr = cfg->spiceListen;
if (listenAddr)
-virBufferAsprintf(, ",addr=%s", listenAddr);
+virBufferAsprintf(, "addr=%s,", listenAddr);
VIR_FREE(netAddr);
if (graphics->data.spice.mousemode) {
switch (graphics->data.spice.mousemode) {
case VIR_DOMAIN_GRAPHICS_SPICE_MOUSE_MODE_SERVER:
-virBufferAddLit(, ",agent-mouse=off");
+virBufferAddLit(, "agent-mouse=off,");
break;
case VIR_DOMAIN_GRAPHICS_SPICE_MOUSE_MODE_CLIENT:
-virBufferAddLit(, ",agent-mouse=on");
+virBufferAddLit(, "agent-mouse=on,");
break;
default:
break;
@@ -7106,18 +7105,19 @@
qemuBuildGraphicsSPICECommandLine(virQEMUDriverConfigPtr cfg,
* making it visible on CLI, so there's no use of password=XXX
* in this bit of the code */
if (!graphics->data.spice.auth.passwd &&
-!cfg->spicePassword)
-virBufferAddLit(, ",disable-ticketing");
+!cfg->spicePassword) {
+virBufferAddLit(, "disable-ticketing,");
+}
if (tlsPort > 0)
-virBufferAsprintf(, ",x509-dir=%s", cfg->spiceTLSx509certdir);
+virBufferAsprintf(, "x509-dir=%s,", cfg->spiceTLSx509certdir);
switch (defaultMode) {
case VIR_DOMAIN_GRAPHICS_SPICE_CHANNEL_MODE_SECURE:
-virBufferAddLit(, ",tls-channel=default");
+virBufferAddLit(, "tls-channel=default,");
break;
case VIR_DOMAIN_GRAPHICS_SPICE_CHANNEL_MODE_INSECURE:
-virBufferAddLit(, ",plaintext-channel=default");
+virBufferAddLit(, "plaintext-channel=default,");
break;
case VIR_DOMAIN_GRAPHICS_SPICE_CHANNEL_MODE_ANY:
/* nothing */
@@ -7133,7 +7133,7 @@ qemuBuildGraphicsSPICECommandLine(virQEMUDriverConfigPtr
cfg,
"but TLS port is not provided"));
goto error;
}
-virBufferAsprintf(, ",tls-channel=%s",
+virBufferAsprintf(, "tls-channel=%s,",
virDomainGraphicsSpiceChannelNameTypeToString(i));
break;
@@ -7144,7 +7144,7 @@ qemuBuildGraphicsSPICECommandLine(virQEMUDriverConfigPtr
cfg,
"configuration, but plain port is not
provided"));
goto error;
}
-virBufferAsprintf(, ",plaintext-channel=%s",
+virBufferAsprintf(, "plaintext-channel=%s,",
virDomainGraphicsSpiceChannelNameTypeToString(i));
break;
@@ -7175,30 +7175,36 @@
qemuBuildGraphicsSPICECommandLine(virQEMUDriverConfigPtr cfg,
}
}
-if (graphics->data.spice.image)
-virBufferAsprintf(, ",image-compression=%s",
+if (graphics->data.spice.image) {
+virBufferAsprintf(, "image-compression=%s,",
virDomainGraphicsSpiceImageCompressionTypeToString(graphics->data.spice.image));
-if (graphics->data.spice.jpeg)
-virBufferAsprintf(, ",jpeg-wan-compression=%s",
+}
+if (graphics->data.spice.jpeg) {
+virBufferAsprintf(, "jpeg-wan-compression=%s,",
virDomainGraphicsSpiceJpegCompressionTypeToString(graphics->data.spice.jpeg));
-if (graphics->data.spice.zlib)
Re: [libvirt] [PATCH v2 1/3] qemu: Make all SPICE command-line args optional
On Wed, Mar 16, 2016 at 05:45:03PM +0100, Christophe Fergeau wrote:
> The end goal is to avoid adding -spice port=0,addr=127.0.0.1 to QEMU command
> line when no SPICE port is specified in libvirt XML.
>
> Currently, the code relies on port=xx to always be present, so subsequent
> args can be unconditionally appended with a leading ','. Since port=0
> will no longer be added in a subsequent commit, we append a ',' to every
> arg instead of prepending, and remove the last one before adding it to
> the arg list.
> ---
> src/qemu/qemu_command.c | 68
> +++--
> 1 file changed, 38 insertions(+), 30 deletions(-)
ACK
> -if (graphics->data.spice.playback)
> -virBufferAsprintf(, ",playback-compression=%s",
> +}
> +if (graphics->data.spice.playback) {
> +virBufferAsprintf(, "playback-compression=%s,",
>
> virTristateSwitchTypeToString(graphics->data.spice.playback));
> -if (graphics->data.spice.streaming)
> -virBufferAsprintf(, ",streaming-video=%s",
> +}
> +if (graphics->data.spice.streaming) {
> +virBufferAsprintf(, "streaming-video=%s,",
>
> virDomainGraphicsSpiceStreamingModeTypeToString(graphics->data.spice.streaming));
> -if (graphics->data.spice.copypaste == VIR_TRISTATE_BOOL_NO)
> -virBufferAddLit(, ",disable-copy-paste");
> +}
> +if (graphics->data.spice.copypaste == VIR_TRISTATE_BOOL_NO) {
> +virBufferAddLit(, "disable-copy-paste,");
> +}
This breaks make syntax-check:
Curly brackets around single-line body:
src/qemu/qemu_command.c:7559-7561:
if (graphics->data.spice.copypaste == VIR_TRISTATE_BOOL_NO) {
virBufferAddLit(, "disable-copy-paste,");
}
Jan
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 1/3] qemu: Make all SPICE command-line args optional
On Fri, Mar 18, 2016 at 10:25:58AM +0100, Ján Tomko wrote:
> > -if (graphics->data.spice.playback)
> > -virBufferAsprintf(, ",playback-compression=%s",
> > +}
> > +if (graphics->data.spice.playback) {
> > +virBufferAsprintf(, "playback-compression=%s,",
> >
> > virTristateSwitchTypeToString(graphics->data.spice.playback));
> > -if (graphics->data.spice.streaming)
> > -virBufferAsprintf(, ",streaming-video=%s",
> > +}
> > +if (graphics->data.spice.streaming) {
> > +virBufferAsprintf(, "streaming-video=%s,",
> >
> > virDomainGraphicsSpiceStreamingModeTypeToString(graphics->data.spice.streaming));
> > -if (graphics->data.spice.copypaste == VIR_TRISTATE_BOOL_NO)
> > -virBufferAddLit(, ",disable-copy-paste");
> > +}
> > +if (graphics->data.spice.copypaste == VIR_TRISTATE_BOOL_NO) {
> > +virBufferAddLit(, "disable-copy-paste,");
> > +}
>
> This breaks make syntax-check:
>
> Curly brackets around single-line body:
> src/qemu/qemu_command.c:7559-7561:
> if (graphics->data.spice.copypaste == VIR_TRISTATE_BOOL_NO) {
> virBufferAddLit(, "disable-copy-paste,");
> }
Ah thanks, I'll make sure to fix these before pushing. Totally forgot to
rerun it before sending v2.
Christophe
signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
