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

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.

Signed-off-by: Chris Venteicher <cventeic(a)redhat.com&gt; --- 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"

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

+ 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);

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

+ + +/** + * 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.

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

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