10:47 a.m.
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(a)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