[libvirt] [PATCH 1/3] qemu: Make all SPICE command-line args optional

[Christophe Fergeau]

Hey,

On Tue, Mar 15, 2016 at 12:16:10PM +0100, Ján Tomko wrote:
> On Tue, Mar 15, 2016 at 10:20:48AM +0100, Christophe Fergeau wrote:
> > The goal is to not add -spice port=0,addr=127.0.0.1 to QEMU command line
> > when no SPICE port is specified in libvirt XML.
> 
> Misleading, even after the series port=0 is generated.

Indeed, I'll reword this.

> 
> > Before this change, we could rely on port or tlsPort to always be
> > present, so subsequent args could be unconditionally appended with a
> > leading ','. Now that it's no longer the case, we need to always check
> > whether the arg we are about to append is the first one or not.

Even that part is not correct before the last patch in the series, I'll
reword it too.

> > ---
> >  src/qemu/qemu_command.c | 98 ++++++++++++++++++++++++++++++++++++-------------
> >  1 file changed, 73 insertions(+), 25 deletions(-)
> > 
> > diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> > index 32d32b1..84db056 100644
> > --- a/src/qemu/qemu_command.c
> > +++ b/src/qemu/qemu_command.c
> > @@ -7039,13 +7039,16 @@ qemuBuildGraphicsSPICECommandLine(virQEMUDriverConfigPtr cfg,
> >                               " but TLS is disabled in qemu.conf"));
> >              goto error;
> >          }
> > -        if (port > 0)
> > +
> > +        if (virBufferUse(&opt) != 0)
> >              virBufferAddChar(&opt, ',');
> >          virBufferAsprintf(&opt, "tls-port=%u", tlsPort);
> 
> Instead of all these conditions, we can convert all the arguments to add
> a trailing comma and remove the last one by calling
>     virBufferTrim(&opt, ",", -1);
> at the end.

Ah indeed, I knew there had to be a better way than adding these checks
all over the place. Thanks a lot, I'll change it :)

> 
> >      }
> >  
> 
> > +    /* If we did not add any SPICE arguments, add a dummy 'port=0' one
> > +     * as -spice alone is not allowed on QEMU command line and will be
> > +     * ignored by libvirt
> > +     */
> > +    if (virBufferUse(&opt) == 0)
> > +      virBufferAddLit(&opt, "port=0");
> > +
> 
> I would rather add this change to the commit that disables the first
> port=0.

Ok, I was torn between adding it here or in the commit you mention, I'll
move it :)

> 
> Also, there is no need to go through the buffer, the string can be added
> directly to the virCommand.

Ah ok, thanks.

I'll send a v2!

Christophe
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20160316/2fb764ab/attachment-0001.sig>