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@xxxxxxxxxx>
> > ---
> > 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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
