Re: [libvirt] [PATCH 03/10] qemu: Make basic upfront checks before creating command
On 02/17/2016 03:13 AM, Peter Krempa wrote:
On Tue, Feb 16, 2016 at 19:44:13 -0500, John Ferlan wrote:
> Create qemuBuildPreCheck to make some basic upfront checks before
> trying to build the command.
>
> Unfortunately the 'spice' count was used later on, so yuck, handle that.
The count is not needed. Just the fact that there are spice devices is
required. Additionally I don't understand why spice-transported serial
ports are skipped rather than pointed out if there is no spice graphics
to transport them. I'd fix that one first rather than having to deal
with the pointer and having a checker that needs to leak stuff out to
the caller.
Later in the code, they are just ignored:
for (i = 0; i < def->nserials; i++) {
virDomainChrDefPtr serial = def->serials[i];
char *devstr;
if (serial->source.type == VIR_DOMAIN_CHR_TYPE_SPICEPORT &&
!spicecnt)
continue;
I'm not familiar with a spiceport - so I left things as is except of
course for keeping track that there is one.
Looks like the code was introduced by Martin (commit id 'd27e6bc40') and
it's designed that way. There's even a test for it.
So it seems my alternative is to perform that same ngraphics loop later
on either as a precursor to the nserials loop or within the loop only if
a SPICEPORT was found. Or of course leak/return a bool hasSpice.
>
> This will move some logic from much later to much earlier - we shouldn't
> be adjusting any data so that shouldn't matter.
>
> Signed-off-by: John Ferlan <jferlan(a)redhat.com>
> ---
> src/qemu/qemu_command.c | 169 +++++++++++++++++++++++++++---------------------
> 1 file changed, 97 insertions(+), 72 deletions(-)
>
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index ab27619..36fe110 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -6593,6 +6593,99 @@ qemuBuildTPMCommandLine(virDomainDefPtr def,
> }
>
>
> +/*
> + * qemuBuildPreCheck:
I'd prefer qemuBuildCommandLineValidate
> + *
> + * Perform some basic configuration checks before taking the plunge.
And something either more descriptive or at least finishing the sentence
after 'checks'.
> + */
> +static int
> +qemuBuildPreCheck(virQEMUDriverPtr driver,
> + const virDomainDef *def,
> + int *spicecnt)
Checks should not leak any value.
> +{
> + size_t i;
> + int sdl = 0;
> + int vnc = 0;
> + int spice = 0;
[...]
> +
> +
> + return 0;
> +
> + error:
> + return -1;
Nothing to clean up, thus the label doesn't make sense.
OK - rather than goto error's, I'll replace with return -1 (6 of one,
half dozen of another)
> +}
> +
> +
> qemuBuildCommandLineCallbacks buildCommandLineCallbacks = {
> .qemuGetSCSIDeviceSgName = virSCSIDeviceGetSgName,
> };
[...]
> @@ -6677,47 +6768,8 @@ qemuBuildCommandLine(virConnectPtr conn,
> -
> - for (i = 0; i < def->ngraphics; ++i) {
> - switch (def->graphics[i]->type) {
> - case VIR_DOMAIN_GRAPHICS_TYPE_SDL:
> - ++sdl;
> - break;
> - case VIR_DOMAIN_GRAPHICS_TYPE_VNC:
> - ++vnc;
> - break;
> - case VIR_DOMAIN_GRAPHICS_TYPE_SPICE:
> - ++spice;
> - break;
> - }
> - }
> + if (qemuBuildPrecCheck(driver, def, &spicecnt) < 0)
This won't compile.
Ugh... My bad - I had a different and much worse name, changed it at the
last second - at least it was not a change name, then push, then leave
for the day ;-)
Tks -
John