[libvirt] [PATCH v2 09/11] rpc: refactor virNetServer setup for post-exec restarts

With the current code it is neccessary to call virNetDaemonNewPostExecRestart() and then for each server that needs restarting you are supposed to call virNetDaemonAddSeverPostExecRestart() This is fine if there's only ever one server, but as soon as you have two servers it is impossible to use this design. The code has no idea which servers were recorded in the JSON state doc, nor in which order the hash table serialized its keys. So this patch changes things so that we only call virNetDaemonNewPostExecRestart() passing in a callback, which is invoked once for each server found int he JSON state doc. Signed-off-by: Daniel P. Berrange <berrange(a)redhat.com&gt; --- src/libvirt_remote.syms | 1 - src/locking/lock_daemon.c | 41 +++++++++---- src/logging/log_daemon.c | 41 +++++++++---- src/rpc/virnetdaemon.c | 151 ++++++++++++++++++++++++---------------------- src/rpc/virnetdaemon.h | 18 +++--- tests/virnetdaemontest.c | 37 ++++++++++-- 6 files changed, 177 insertions(+), 112 deletions(-) diff --git a/src/libvirt_remote.syms b/src/libvirt_remote.syms index fab6ab9dff..b75cbb99b2 100644 --- a/src/libvirt_remote.syms +++ b/src/libvirt_remote.syms @@ -61,7 +61,6 @@ virNetClientStreamSetError; # rpc/virnetdaemon.h virNetDaemonAddServer; -virNetDaemonAddServerPostExec; virNetDaemonAddShutdownInhibition; virNetDaemonAddSignalHandler; virNetDaemonAutoShutdown; diff --git a/src/locking/lock_daemon.c b/src/locking/lock_daemon.c index 6751b57bc5..b1f0665aaa 100644 --- a/src/locking/lock_daemon.c +++ b/src/locking/lock_daemon.c @@ -192,15 +192,38 @@ virLockDaemonNew(virLockDaemonConfigPtr config, bool privileged) } +static virNetServerPtr +virLockDaemonNewServerPostExecRestart(virNetDaemonPtr dmn ATTRIBUTE_UNUSED, + const char *name, + virJSONValuePtr object, + void *opaque) +{ + if (STREQ(name, "virtlockd")) { + return virNetServerNewPostExecRestart(object, + name, + virLockDaemonClientNew, + virLockDaemonClientNewPostExecRestart, + virLockDaemonClientPreExecRestart, + virLockDaemonClientFree, + opaque); + } else { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Unexpected server name '%s' during restart"), + name); + return NULL; + } +} + + static virLockDaemonPtr virLockDaemonNewPostExecRestart(virJSONValuePtr object, bool privileged) { virLockDaemonPtr lockd; virJSONValuePtr child; virJSONValuePtr lockspaces; - virNetServerPtr srv; size_t i; ssize_t n; + const char *serverNames[] = { "virtlockd" }; if (VIR_ALLOC(lockd) < 0) return NULL; @@ -267,18 +290,12 @@ virLockDaemonNewPostExecRestart(virJSONValuePtr object, bool privileged) } } - if (!(lockd->dmn = virNetDaemonNewPostExecRestart(child))) - goto error; - - if (!(srv = virNetDaemonAddServerPostExec(lockd->dmn, - "virtlockd", - virLockDaemonClientNew, - virLockDaemonClientNewPostExecRestart, - virLockDaemonClientPreExecRestart, - virLockDaemonClientFree, - (void*)(intptr_t)(privileged ? 0x1 : 0x0)))) + if (!(lockd->dmn = virNetDaemonNewPostExecRestart(child, + ARRAY_CARDINALITY(serverNames), + serverNames, + virLockDaemonNewServerPostExecRestart, + (void*)(intptr_t)(privileged ? 0x1 : 0x0)))) goto error; - virObjectUnref(srv); return lockd; diff --git a/src/logging/log_daemon.c b/src/logging/log_daemon.c index 7e8c9cfc29..33133af2af 100644 --- a/src/logging/log_daemon.c +++ b/src/logging/log_daemon.c @@ -188,13 +188,36 @@ virLogDaemonGetHandler(virLogDaemonPtr dmn) } +static virNetServerPtr +virLogDaemonNewServerPostExecRestart(virNetDaemonPtr dmn ATTRIBUTE_UNUSED, + const char *name, + virJSONValuePtr object, + void *opaque) +{ + if (STREQ(name, "virtlogd")) { + return virNetServerNewPostExecRestart(object, + name, + virLogDaemonClientNew, + virLogDaemonClientNewPostExecRestart, + virLogDaemonClientPreExecRestart, + virLogDaemonClientFree, + opaque); + } else { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Unexpected server name '%s' during restart"), + name); + return NULL; + } +} + + static virLogDaemonPtr virLogDaemonNewPostExecRestart(virJSONValuePtr object, bool privileged, virLogDaemonConfigPtr config) { virLogDaemonPtr logd; - virNetServerPtr srv; virJSONValuePtr child; + const char *serverNames[] = { "virtlogd" }; if (VIR_ALLOC(logd) < 0) return NULL; @@ -212,18 +235,12 @@ virLogDaemonNewPostExecRestart(virJSONValuePtr object, bool privileged, goto error; } - if (!(logd->dmn = virNetDaemonNewPostExecRestart(child))) - goto error; - - if (!(srv = virNetDaemonAddServerPostExec(logd->dmn, - "virtlogd", - virLogDaemonClientNew, - virLogDaemonClientNewPostExecRestart, - virLogDaemonClientPreExecRestart, - virLogDaemonClientFree, - (void*)(intptr_t)(privileged ? 0x1 : 0x0)))) + if (!(logd->dmn = virNetDaemonNewPostExecRestart(child, + ARRAY_CARDINALITY(serverNames), + serverNames, + virLogDaemonNewServerPostExecRestart, + (void*)(intptr_t)(privileged ? 0x1 : 0x0)))) goto error; - virObjectUnref(srv); if (!(child = virJSONValueObjectGet(object, "handler"))) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", diff --git a/src/rpc/virnetdaemon.c b/src/rpc/virnetdaemon.c index 5d61a255c6..6f00bfd9d1 100644 --- a/src/rpc/virnetdaemon.c +++ b/src/rpc/virnetdaemon.c @@ -262,85 +262,38 @@ virNetDaemonGetServers(virNetDaemonPtr dmn, } -virNetServerPtr -virNetDaemonAddServerPostExec(virNetDaemonPtr dmn, - const char *serverName, - virNetServerClientPrivNew clientPrivNew, - virNetServerClientPrivNewPostExecRestart clientPrivNewPostExecRestart, - virNetServerClientPrivPreExecRestart clientPrivPreExecRestart, - virFreeCallback clientPrivFree, - void *clientPrivOpaque) -{ - virJSONValuePtr object = NULL; - virNetServerPtr srv = NULL; - - virObjectLock(dmn); - - if (!dmn->srvObject) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("Cannot add more servers post-exec than " - "there were pre-exec")); - goto error; - } - - if (virJSONValueIsArray(dmn->srvObject)) { - object = virJSONValueArraySteal(dmn->srvObject, 0); - if (virJSONValueArraySize(dmn->srvObject) == 0) { - virJSONValueFree(dmn->srvObject); - dmn->srvObject = NULL; - } - } else if (virJSONValueObjectGetByType(dmn->srvObject, - "min_workers", - VIR_JSON_TYPE_NUMBER)) { - object = dmn->srvObject; - dmn->srvObject = NULL; - } else { - int ret = virJSONValueObjectRemoveKey(dmn->srvObject, - serverName, - &object); - if (ret != 1) { - if (ret == 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Server '%s' not found in JSON"), serverName); - } - goto error; - } - - if (virJSONValueObjectKeysNumber(dmn->srvObject) == 0) { - virJSONValueFree(dmn->srvObject); - dmn->srvObject = NULL; - } - } +struct virNetDaemonServerData { + virNetDaemonPtr dmn; + virNetDaemonNewServerPostExecRestart cb; + void *opaque; +}; - srv = virNetServerNewPostExecRestart(object, - serverName, - clientPrivNew, - clientPrivNewPostExecRestart, - clientPrivPreExecRestart, - clientPrivFree, - clientPrivOpaque); +static int +virNetDaemonServerIterator(const char *key, + virJSONValuePtr value, + void *opaque) +{ + struct virNetDaemonServerData *data = opaque; + virNetServerPtr srv; + VIR_DEBUG("Creating server '%s'", key); + srv = data->cb(data->dmn, key, value, data->opaque); if (!srv) - goto error; - - if (virHashAddEntry(dmn->servers, serverName, srv) < 0) - goto error; - virObjectRef(srv); + return -1; - virJSONValueFree(object); - virObjectUnlock(dmn); - return srv; + if (virHashAddEntry(data->dmn->servers, key, srv) < 0) + return -1; - error: - virObjectUnlock(dmn); - virObjectUnref(srv); - virJSONValueFree(object); - return NULL; + return 0; } virNetDaemonPtr -virNetDaemonNewPostExecRestart(virJSONValuePtr object) +virNetDaemonNewPostExecRestart(virJSONValuePtr object, + size_t nDefServerNames, + const char **defServerNames, + virNetDaemonNewServerPostExecRestart cb, + void *opaque) { virNetDaemonPtr dmn = NULL; virJSONValuePtr servers = virJSONValueObjectGet(object, "servers"); @@ -355,10 +308,64 @@ virNetDaemonNewPostExecRestart(virJSONValuePtr object) goto error; } - if (!(dmn->srvObject = virJSONValueCopy(new_version ? servers : object))) - goto error; + if (!new_version) { + virNetServerPtr srv; + + if (nDefServerNames < 1) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("No default server names provided")); + goto error; + } + + VIR_DEBUG("No 'servers' data, creating default '%s' name", defServerNames[0]); + + srv = cb(dmn, defServerNames[0], object, opaque); + + if (virHashAddEntry(dmn->servers, defServerNames[0], srv) < 0) + goto error; + } else if (virJSONValueIsArray(servers)) { + size_t i; + ssize_t n = virJSONValueArraySize(servers); + if (n < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Server count %zd should be positive"), n); + goto error; + } + if (n > nDefServerNames) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Server count %zd greater than default name count %zu"), + n, nDefServerNames); + goto error; + } + + for (i = 0; i < n; i++) { + virNetServerPtr srv; + virJSONValuePtr value = virJSONValueArrayGet(servers, i); + + VIR_DEBUG("Creating server '%s'", defServerNames[i]); + srv = cb(dmn, defServerNames[i], value, opaque); + if (!srv) + goto error; + + if (virHashAddEntry(dmn->servers, defServerNames[i], srv) < 0) { + virObjectUnref(srv); + goto error; + } + } + } else { + struct virNetDaemonServerData data = { + dmn, + cb, + opaque, + }; + if (virJSONValueObjectForeachKeyValue(servers, + virNetDaemonServerIterator, + &data) < 0) + goto error; + } return dmn; + error: virObjectUnref(dmn); return NULL; diff --git a/src/rpc/virnetdaemon.h b/src/rpc/virnetdaemon.h index 72c1df69b4..6576c463b5 100644 --- a/src/rpc/virnetdaemon.h +++ b/src/rpc/virnetdaemon.h @@ -40,15 +40,15 @@ virNetDaemonPtr virNetDaemonNew(void); int virNetDaemonAddServer(virNetDaemonPtr dmn, virNetServerPtr srv); -virNetServerPtr virNetDaemonAddServerPostExec(virNetDaemonPtr dmn, - const char *serverName, - virNetServerClientPrivNew clientPrivNew, - virNetServerClientPrivNewPostExecRestart clientPrivNewPostExecRestart, - virNetServerClientPrivPreExecRestart clientPrivPreExecRestart, - virFreeCallback clientPrivFree, - void *clientPrivOpaque); - -virNetDaemonPtr virNetDaemonNewPostExecRestart(virJSONValuePtr object); +typedef virNetServerPtr (*virNetDaemonNewServerPostExecRestart)(virNetDaemonPtr dmn, + const char *name, + virJSONValuePtr object, + void *opaque); +virNetDaemonPtr virNetDaemonNewPostExecRestart(virJSONValuePtr object, + size_t nDefServerNames, + const char **defServerNames, + virNetDaemonNewServerPostExecRestart cb, + void *opaque); virJSONValuePtr virNetDaemonPreExecRestart(virNetDaemonPtr dmn); diff --git a/tests/virnetdaemontest.c b/tests/virnetdaemontest.c index 3e60f09007..435513d314 100644 --- a/tests/virnetdaemontest.c +++ b/tests/virnetdaemontest.c @@ -194,12 +194,32 @@ struct testExecRestartData { bool pass; }; +static virNetServerPtr +testNewServerPostExecRestart(virNetDaemonPtr dmn ATTRIBUTE_UNUSED, + const char *name, + virJSONValuePtr object, + void *opaque) +{ + struct testExecRestartData *data = opaque; + size_t i; + for (i = 0; i < data->nservers; i++) { + if (STREQ(data->serverNames[i], name)) { + return virNetServerNewPostExecRestart(object, + name, + NULL, NULL, NULL, + NULL, NULL); + } + } + + virReportError(VIR_ERR_INTERNAL_ERROR, "Unexpected server name '%s'", name); + return NULL; +} + static int testExecRestart(const void *opaque) { size_t i; int ret = -1; virNetDaemonPtr dmn = NULL; - virNetServerPtr srv = NULL; const struct testExecRestartData *data = opaque; char *infile = NULL, *outfile = NULL; char *injsonstr = NULL, *outjsonstr = NULL; @@ -241,15 +261,20 @@ static int testExecRestart(const void *opaque) if (!(injson = virJSONValueFromString(injsonstr))) goto cleanup; - if (!(dmn = virNetDaemonNewPostExecRestart(injson))) + if (!(dmn = virNetDaemonNewPostExecRestart(injson, + data->nservers, + data->serverNames, + testNewServerPostExecRestart, + (void *)data))) goto cleanup; for (i = 0; i < data->nservers; i++) { - if (!(srv = virNetDaemonAddServerPostExec(dmn, data->serverNames[i], - NULL, NULL, NULL, - NULL, NULL))) + if (!virNetDaemonHasServer(dmn, data->serverNames[i])) { + virReportError(VIR_ERR_INTERNAL_ERROR, + "Server %s was not created", + data->serverNames[i]); goto cleanup; - srv = NULL; + } } if (!(outjson = virNetDaemonPreExecRestart(dmn))) -- 2.14.3