On 12/20/2017 11:47 AM, Daniel P. Berrange wrote:
> Signed-off-by: Daniel P. Berrange <berrange@xxxxxxxxxx>
> ---
> src/qemu/qemu_driver.c | 57 +++++++++++++++++++++++++++++++++++++++++++++++++
> src/qemu/qemu_process.c | 31 ++++++++++++++++++---------
> src/qemu/qemu_process.h | 1 +
> 3 files changed, 79 insertions(+), 10 deletions(-)
>
It feels like there's two separate patches here as the qemu_process
stuff seems separable.
Of course it dawns on me now... Should we go with a QemuConnect type
naming scheme? Usage of Reconnect would seem to assume we lost
connection at some point because libvirtd was stopped or crashed. Of
course Attach would have been "optimal", but that's already used for
something else.
Perhaps the "proof" that things can work is to be able to have the
process reconnect logic and this logic intersect. Theoretically they are
doing the same thing in the long run at least with respect to being able
to connect to an existing qemu process.
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 97b194b057..fea1f24250 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -16300,6 +16300,62 @@ static virDomainPtr qemuDomainQemuAttach(virConnectPtr conn,
> }
>
>
> +static virDomainPtr qemuDomainQemuReconnect(virConnectPtr conn,
> + const char *name,
> + unsigned int flags)
static virDomainPtr
qemuDomainQemuReconnect(...)
> +{
> + virQEMUDriverPtr driver = conn->privateData;
> + virDomainObjPtr vm = NULL;
> + virDomainPtr dom = NULL;
> + virCapsPtr caps = NULL;
> + virQEMUDriverConfigPtr cfg;
> +
> + virCheckFlags(0, NULL);
> +
> + cfg = virQEMUDriverGetConfig(driver);
> +
> + if (strchr(name, '/')) {
> + virReportError(VIR_ERR_XML_ERROR,
> + _("name %s cannot contain '/'"), name);
> + goto cleanup;
> + }
> +
> + vm = virDomainObjListFindByName(driver->domains, name);
> + if (vm) {
> + virReportError(VIR_ERR_OPERATION_INVALID,
> + _("Domain '%s' already exists"), name);
> + goto cleanup;
> + }
> +
> + if (!(caps = virQEMUDriverGetCapabilities(driver, false)))
> + goto cleanup;
> +
> + if (!(vm = virDomainObjListLoadStatus(driver->domains,
> + cfg->stateDir,
> + name,
> + caps,
> + driver->xmlopt,
> + NULL, NULL))) {
> + goto cleanup;
> + }
This would assume that stateDir has a file @name.xml, right? IOW: shim
processing creates it. I only mention it as a "one extra sanity" type
check that we may want to make; otherwise, we could get some sort of
less obvious error... I didn't do much digging to prove/disprove.
> +
> + if (virDomainQemuReconnectEnsureACL(conn, vm->def) < 0) {
> + qemuDomainRemoveInactive(driver, vm);
> + goto cleanup;
> + }
> +
> + dom = virGetDomain(conn, vm->def->name, vm->def->uuid, vm->def->id);
> +
> + qemuDomainObjEndJob(driver, vm);
When did we Begin the job? I assume that this is borrowing logic from
qemuProcessReconnect and this may just be "leftover".
> +
> + cleanup:
> + virDomainObjEndAPI(&vm);
> + virObjectUnref(caps);
> + virObjectUnref(cfg);
> + return dom;
> +}
> +
> +
> static int
> qemuDomainOpenConsole(virDomainPtr dom,
> const char *dev_name,
> @@ -21262,6 +21318,7 @@ static virHypervisorDriver qemuHypervisorDriver = {
> .domainSnapshotDelete = qemuDomainSnapshotDelete, /* 0.8.0 */
> .domainQemuMonitorCommand = qemuDomainQemuMonitorCommand, /* 0.8.3 */
> .domainQemuAttach = qemuDomainQemuAttach, /* 0.9.4 */
> + .domainQemuReconnect = qemuDomainQemuReconnect, /* 4.1.0 */
> .domainQemuAgentCommand = qemuDomainQemuAgentCommand, /* 0.10.0 */
> .connectDomainQemuMonitorEventRegister = qemuConnectDomainQemuMonitorEventRegister, /* 1.2.3 */
> .connectDomainQemuMonitorEventDeregister = qemuConnectDomainQemuMonitorEventDeregister, /* 1.2.3 */
> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> index a0f430f89f..ea924cf9b6 100644
> --- a/src/qemu/qemu_process.c
> +++ b/src/qemu/qemu_process.c
> @@ -7088,14 +7088,10 @@ struct qemuProcessReconnectData {
> * We can't do normal MonitorEnter & MonitorExit because these two lock the
> * monitor lock, which does not exists in this early phase.
> */
> -static void
> -qemuProcessReconnect(void *opaque)
> +int
> +qemuProcessReconnect(virQEMUDriverPtr driver, virDomainObjPtr obj, virConnectPtr conn)
One argument per line...
> {
> - struct qemuProcessReconnectData *data = opaque;
> - virQEMUDriverPtr driver = data->driver;
> - virDomainObjPtr obj = data->obj;
> qemuDomainObjPrivatePtr priv;
> - virConnectPtr conn = data->conn;
> struct qemuDomainJobObj oldjob;
> int state;
> int reason;
> @@ -7104,8 +7100,7 @@ qemuProcessReconnect(void *opaque)
> unsigned int stopFlags = 0;
> bool jobStarted = false;
> virCapsPtr caps = NULL;
> -
> - VIR_FREE(data);
> + int ret = -1;
>
> qemuDomainObjRestoreJob(obj, &oldjob);
> if (oldjob.asyncJob == QEMU_ASYNC_JOB_MIGRATION_IN)
> @@ -7297,6 +7292,7 @@ qemuProcessReconnect(void *opaque)
> if (virAtomicIntInc(&driver->nactive) == 1 && driver->inhibitCallback)
> driver->inhibitCallback(true, driver->inhibitOpaque);
>
> + ret = 0;
> cleanup:
> if (jobStarted) {
> if (!virDomainObjIsActive(obj))
> @@ -7311,7 +7307,7 @@ qemuProcessReconnect(void *opaque)
> virObjectUnref(cfg);
> virObjectUnref(caps);
> virNWFilterUnlockFilterUpdates();
> - return;
> + return ret;
>
> error:
> if (virDomainObjIsActive(obj)) {
> @@ -7338,6 +7334,21 @@ qemuProcessReconnect(void *opaque)
> goto cleanup;
> }
>
> +
> +static void
> +qemuProcessReconnectThread(void *opaque)
> +{
> + struct qemuProcessReconnectData *data = opaque;
> + virQEMUDriverPtr driver = data->driver;
> + virDomainObjPtr obj = data->obj;
> + virConnectPtr conn = data->conn;
> +
> + qemuProcessReconnect(driver, obj, conn);
So we modify the above call to return an int, but do nothing with that
status. I suspect there is a future reason for qemuProcessReconnect, but
figured I'd at least note it...
John
> +
> + VIR_FREE(data);
> +}
> +
> +
> static int
> qemuProcessReconnectHelper(virDomainObjPtr obj,
> void *opaque)
> @@ -7369,7 +7380,7 @@ qemuProcessReconnectHelper(virDomainObjPtr obj,
> */
> virObjectRef(data->conn);
>
> - if (virThreadCreate(&thread, false, qemuProcessReconnect, data) < 0) {
> + if (virThreadCreate(&thread, false, qemuProcessReconnectThread, data) < 0) {
> virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> _("Could not create thread. QEMU initialization "
> "might be incomplete"));
> diff --git a/src/qemu/qemu_process.h b/src/qemu/qemu_process.h
> index cd9a720313..846577d341 100644
> --- a/src/qemu/qemu_process.h
> +++ b/src/qemu/qemu_process.h
> @@ -45,6 +45,7 @@ int qemuProcessBuildDestroyMemoryPaths(virQEMUDriverPtr driver,
>
> void qemuProcessAutostartAll(virQEMUDriverPtr driver);
> void qemuProcessReconnectAll(virConnectPtr conn, virQEMUDriverPtr driver);
> +int qemuProcessReconnect(virQEMUDriverPtr driver, virDomainObjPtr obj, virConnectPtr conn);
>
> typedef struct _qemuProcessIncomingDef qemuProcessIncomingDef;
> typedef qemuProcessIncomingDef *qemuProcessIncomingDefPtr;
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
