[libvirt] [PATCH RFC 10/22] qemu_process: Introduce qemuProcessStartQmp

[Michal Privoznik]

On 11/11/2018 08:59 PM, Chris Venteicher wrote:
> Move a step closer to the function structure used elsewhere in
> qemu_process where qemuProcessStart and qemuProcessStop are the exposed
> functions.
> 
> qemuProcessStartQmp mirrors qemuProcessStart in calling sub functions to
> intialize, launch the process and connect the monitor to the QEMU
> process.
> 
> static functions qemuProcessInitQmp, qemuProcessLaunchQmp and
> qemuConnectMonitorQmp are also introduced.
> 
> qemuProcessLaunchQmp is just renamed from qemuProcessRun and
> encapsulates all of the original code.
> 
> qemuProcessInitQmp and qemuProcessMonitorQmp are introduced as empty
> wrappers into which subsequent patches will partition code from
> qemuProcessLaunchQmp.
> 
> Signed-off-by: Chris Venteicher <cventeic at redhat.com>
> ---
>  src/qemu/qemu_capabilities.c |  4 +-
>  src/qemu/qemu_process.c      | 96 +++++++++++++++++++++++++++++++++++-
>  src/qemu/qemu_process.h      |  2 +-
>  3 files changed, 97 insertions(+), 5 deletions(-)
> 
> diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
> index fbb4336201..7168c470f6 100644
> --- a/src/qemu/qemu_capabilities.c
> +++ b/src/qemu/qemu_capabilities.c
> @@ -4230,7 +4230,7 @@ virQEMUCapsInitQMP(virQEMUCapsPtr qemuCaps,
>          goto cleanup;
>  
>  
> -    if (qemuProcessRun(proc) < 0)
> +    if (qemuProcessStartQmp(proc) < 0)
>          goto cleanup;
>  
>      if (!(mon = QEMU_PROCESS_MONITOR(proc))) {
> @@ -4249,7 +4249,7 @@ virQEMUCapsInitQMP(virQEMUCapsPtr qemuCaps,
>          forceTCG = true;
>          proc_kvm = qemuProcessNew(qemuCaps->binary, libDir, runUid, runGid, forceTCG);
>  
> -        if (qemuProcessRun(proc_kvm) < 0)
> +        if (qemuProcessStartQmp(proc_kvm) < 0)
>              goto cleanup;
>  
>          if (!(mon_kvm = QEMU_PROCESS_MONITOR(proc_kvm))) {
> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> index 2640ec2b32..b6aa3a9af3 100644
> --- a/src/qemu/qemu_process.c
> +++ b/src/qemu/qemu_process.c
> @@ -8148,8 +8148,29 @@ qemuProcessNew(const char *binary,
>  }
>  
>  
> -int
> -qemuProcessRun(qemuProcessPtr proc){
> +/* Initialize configuration and paths prior to starting QEMU
> + */
> +static int
> +qemuProcessInitQmp(qemuProcessPtr proc)
> +{
> +    int ret = -1;
> +
> +    VIR_DEBUG("Beginning VM startup process"
> +              "emulator=%s",
> +              proc->binary);
> +
> +    ret = 0;
> +
> +    VIR_DEBUG("ret=%i", ret);
> +    return ret;
> +}
> +

I am sorry, but I'm failing to see the purpose of this function.

> +
> +/* Launch QEMU Process
> + */
> +static int
> +qemuProcessLaunchQmp(qemuProcessPtr proc)
> +{
>      virDomainXMLOptionPtr xmlopt = NULL;
>      const char *machine;
>      int status = 0;
> @@ -8226,6 +8247,77 @@ qemuProcessRun(qemuProcessPtr proc){
>  }
>  
>  
> +/* Connect Monitor to QEMU Process
> + */
> +static int
> +qemuConnectMonitorQmp(qemuProcessPtr proc)
> +{
> +    int ret = -1;
> +
> +    VIR_DEBUG("proc=%p, emulator=%s, proc->pid=%lld",
> +              proc, NULLSTR(proc->binary), (long long) proc->pid);
> +
> +    ret = 0;
> +
> +    VIR_DEBUG("ret=%i", ret);
> +    return ret;
> +}

Or this function. Looking into next patches I can see that you're
extending them. Well, I still think it's not worth introducing empty
functions, just do the rename as you're doing in next patches.

> +
> +
> +/**
> + * qemuProcessStartQmp:
> + * @proc: Stores Process and Connection State
> + *
> + * Start and connect to QEMU binary so QMP queries can be made.
> + *
> + * Usage:
> + *   proc = qemuProcessNew(binary, forceTCG, libDir, runUid, runGid);
> + *   qemuProcessStartQmp(proc);
> + *   mon = QEMU_PROCESS_MONITOR(proc);
> + *   ** Send QMP Queries to QEMU using monitor **
> + *   qemuProcessStopQmp(proc);
> + *   qemuProcessFree(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 qemuProcessStopQmp and qemuProcessFree must be called to cleanup and
> + * free resources, even in activation failure cases.
> + *
> + * Process error output (proc->qmperr) remains available in qemuProcess struct
> + * until qemuProcessFree is called.
> + */
> +int
> +qemuProcessStartQmp(qemuProcessPtr proc)
> +{
> +    int ret = -1;
> +
> +    VIR_DEBUG("emulator=%s",
> +              proc->binary);
> +
> +    if (qemuProcessInitQmp(proc) < 0)
> +        goto cleanup;
> +
> +    if (qemuProcessLaunchQmp(proc) < 0)
> +        goto stop;
> +
> +    if (qemuConnectMonitorQmp(proc) < 0)
> +        goto stop;
> +
> +    ret = 0;
> +
> + cleanup:
> +    VIR_DEBUG("ret=%i", ret);
> +    return ret;
> +
> + stop:
> +    qemuProcessStopQmp(proc);
> +    goto cleanup;
> +}
> +
> +
>  void
>  qemuProcessStopQmp(qemuProcessPtr proc)
>  {
> diff --git a/src/qemu/qemu_process.h b/src/qemu/qemu_process.h
> index 37194e2501..c34ca52ef5 100644
> --- a/src/qemu/qemu_process.h
> +++ b/src/qemu/qemu_process.h
> @@ -246,7 +246,7 @@ qemuProcessPtr qemuProcessNew(const char *binary,
>  
>  void qemuProcessFree(qemuProcessPtr proc);
>  
> -int qemuProcessRun(qemuProcessPtr proc);
> +int qemuProcessStartQmp(qemuProcessPtr proc);
>  
>  void qemuProcessStopQmp(qemuProcessPtr proc);
>  
> 

Michal