[libvirt] [PATCH v5 10/36] qemu_process: Introduce qemuProcessQmpStart

[Jiri Denemark]

On Sun, Dec 02, 2018 at 23:10:04 -0600, Chris Venteicher wrote:
> Move a step closer to the function structure used elsewhere in
> qemu_process where qemuProcessStart and qemuProcessStop are the exposed
> functions.
> 
> qemuProcessQmpStart mirrors qemuProcessStart in calling sub functions to
> initialize, launch the process and connect the monitor to the QEMU
> process.
> 
> static functions qemuProcessQmpInit, qemuProcessQmpLaunch and
> qemuProcessQmpConnectMonitor are introduced.
> 
> qemuProcessQmpLaunch is just renamed from qemuProcessQmpRun and
> encapsulates all of the original code.
> 
> qemuProcessQmpInit and qemuProcessQmpMonitor are nearly empty functions
> acting as placeholders for later patches where blocks of semi-complicated code
> are cut/pasted into these functions without modification
> (hopefully making review easier.)
> 
> Looking forward, the patch series ultimately moves the code into this
> partitioning:
> 
> - qemuProcessQmpInit
>     Becomes the location of ~25 lines of code to create storage
>     directory, in thread safe way, and initialize paths
>     for monpath, monarg and pidfile.
> 
> - qemuProcessQmpLaunch
>     Becomes the location of ~48 lines of code used to create and run the
>     QEMU command.
> 
> - qemuProcessQmpConnectMonitor
>     Becomes the final location of ~58 lines of code used to open and
>     initialize the monitor connection between libvirt and qemu.
> 
> Three smaller, purpose-identifying, functions of ~60 lines or less seem
> better than a single large process "start" function of > 130 lines.
> 
> Being able to compare and contrast between the domain and non-domain
> versions of process code is useful too.  There is some significant
> overlap between what the non-domain and domain functions do.  There is
> also significant additional functionality in the domain functions that
> might be useful in the non-domain functions in the future.
> Possibly there could be sharing between non-domain and
> domain process code in the future but common code would have
> to be carefully extracted from the domain process code (not trivial.)
> 
> Mirroring the domain process code has some value, but
> partitioning the code into logical chunks of < 60 lines
> is the main reason for the static functions.

Since the changes are all hidden inside qemuProcessQmpStart (formerly
called qemuProcessQmpRun) and nothing is calling the new internal
functions, all this refactoring could have been separated from this
series. But since it's already wired in the series, there's no reason to
separate it. Unless serious issues are found in which case it could be
easier to separate the refactoring (and deal with the result) than
fixing the issues. I don't expect such issues, though.

> Signed-off-by: Chris Venteicher <cventeic at redhat.com>
> ---
>  src/qemu/qemu_capabilities.c |  4 +-
>  src/qemu/qemu_process.c      | 94 +++++++++++++++++++++++++++++++++++-
>  src/qemu/qemu_process.h      |  2 +-
>  3 files changed, 95 insertions(+), 5 deletions(-)
> 
> diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
> index 997f8c19d5..ce60648897 100644
> --- a/src/qemu/qemu_capabilities.c
> +++ b/src/qemu/qemu_capabilities.c
> @@ -4265,7 +4265,7 @@ virQEMUCapsInitQMP(virQEMUCapsPtr qemuCaps,
>                                     runUid, runGid, false)))
>          goto cleanup;
>  
> -    if (qemuProcessQmpRun(proc) < 0) {
> +    if (qemuProcessQmpStart(proc) < 0) {
>          if (proc->status != 0)
>              virReportError(VIR_ERR_INTERNAL_ERROR,
>                             _("Failed to probe QEMU binary with QMP: %s"),
> @@ -4288,7 +4288,7 @@ virQEMUCapsInitQMP(virQEMUCapsPtr qemuCaps,
>          procTCG = qemuProcessQmpNew(qemuCaps->binary, libDir,
>                                      runUid, runGid, true);
>  
> -        if (qemuProcessQmpRun(procTCG) < 0)
> +        if (qemuProcessQmpStart(procTCG) < 0)
>              goto cleanup;
>  
>          if (virQEMUCapsInitQMPMonitorTCG(qemuCaps, procTCG->mon) < 0)
> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> index 8ad685f3ea..938d328235 100644
> --- a/src/qemu/qemu_process.c
> +++ b/src/qemu/qemu_process.c
> @@ -8175,8 +8175,28 @@ qemuProcessQmpNew(const char *binary,
>  }
>  
>  
> -int
> -qemuProcessQmpRun(qemuProcessQmpPtr proc)
> +/* Initialize configuration and paths prior to starting QEMU
> + */
> +static int
> +qemuProcessQmpInit(qemuProcessQmpPtr proc)
> +{
> +    int ret = -1;
> +
> +    VIR_DEBUG("Beginning VM startup process"

Too much copy&paste, this function is not about VM startup process.

> +              " proc=%p, emulator=%s",

We usually log function parameters separately in the first debug message
coming from a function.

> +              proc, proc->binary);
> +
> +    ret = 0;
> +
> +    VIR_DEBUG("ret=%i", ret);
> +    return ret;
> +}
> +
> +
> +/* Launch QEMU Process
> + */
> +static int
> +qemuProcessQmpLaunch(qemuProcessQmpPtr proc)
>  {
>      virDomainXMLOptionPtr xmlopt = NULL;
>      const char *machine;
> @@ -8253,6 +8273,76 @@ qemuProcessQmpRun(qemuProcessQmpPtr proc)
>  }
>  
>  
> +/* Connect Monitor to QEMU Process
> + */
> +static int
> +qemuProcessQmpConnectMonitor(qemuProcessQmpPtr proc)
> +{
> +    int ret = -1;
> +
> +    VIR_DEBUG("proc=%p, emulator=%s, proc->pid=%lld",
> +              proc, NULLSTR(proc->binary), (long long)proc->pid);

Why do you expect proc->binary to be NULL here?

> +
> +    ret = 0;
> +
> +    VIR_DEBUG("ret=%i", ret);
> +    return ret;
> +}

In VM startup code monitor connection is initiated inside the *Launch
function. So I'd expect this to be similar if you're trying to mimic
that code. But we'll see, it looks like there could be some benefit in
doing it this way...

> +
> +
> +/**
> + * qemuProcessQmpStart:
> + * @proc: Stores Process and Connection State
> + *
> + * Start and connect to QEMU binary so QMP queries can be made.
> + *
> + * Usage:
> + *   proc = qemuProcessQmpNew(binary, libDir, runUid, runGid, forceTCG);
> + *   qemuProcessQmpStart(proc);
> + *   ** Send QMP Queries to QEMU using monitor (proc->mon) **
> + *   qemuProcessQmpStop(proc);
> + *   qemuProcessQmpFree(proc);
> + *
> + * Check monitor is not NULL before using.
> + *
> + * QEMU probing failure results in monitor being NULL but is not considered
> + * an error case and does not result in a negative return value.
> + *
> + * Both qemuProcessQmpStop and qemuProcessQmpFree must be called to cleanup and
> + * free resources, even in activation failure cases.

IIUC you require the caller to always do qemuProcessQmpStop so why are
you calling it too? And since qemuProcessQmpStop is always (if I'm not
mistaken) used with qemuProcessQmpFree, why don't we just merge the two
functions? The separated functions made sense when the same
qemuProcessQmp structure was reused for several processes, but that
changed several patches ago.

> + *
> + * Process error output (proc->qmperr) remains available in qemuProcessQmp

s/qmperr/stderr/

> + * struct until qemuProcessQmpFree is called.
> + */
> +int
> +qemuProcessQmpStart(qemuProcessQmpPtr proc)
...

Jirka