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

[Chris Venteicher]

Quoting Michal Privoznik (2018-11-14 09:45:07)
> 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.

Yep, I was trying to make it easier to review but didn't explain well
enough from the start. Sorry I wasn't clear.

Patch 10 (this patch) and 12 are about making it possible to do simple
cut/paste moves on semi-complicated blocks of original code moved
within patches 11 and 13.

The goal was to make patches 11 and 13 easy to review because
I don't actually change the code.  It's just moved.

If this seems good with the better explanation I can just try to make
that clear in the commit message for patch 10 and 12.

If it's more confusing this way I can start out with qemuProcesStartQmp
only calling qemuProcessLaunchQmp and the add qemuProcessInitQmp and
qemuConnectMonitorQmp as individual commits with full contents and just
explain that the guts are cut/paste moves with no changes in the commit
message.

Please let me know which approach you think is best.
>
> > +
> > +
> > +/**
> > + * 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