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@redhat.com>

diff --git a/src/libvirt_remote.syms b/src/libvirt_remote.syms
index fab6ab9dff18ec8e0c5ce7e6d110e6fa22f8ee57..b75cbb99b297fcb988afa5269cca7440463b2075 100644 (file)
--- 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 6751b57bc5c9567d09eebe57947399cd4164877b..b1f0665aaa440d9b785eb86035b0f47e9a38c093 100644 (file)
--- 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 7e8c9cfc2926806c84181270458aa0f7e7bdd5c2..33133af2af37f6aeb4dc0aa723d5a8621f5e5d50 100644 (file)
--- 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 5d61a255c6b3a0feff74cb84b3bfd345a1471c06..6f00bfd9d1c91ce9753d1c302fccb2635852ace9 100644 (file)
--- 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 72c1df69b4247d3dca2750d9eeee86cc5e33ba60..6576c463b50edf401bd5d9ca9719dbef563aec0a 100644 (file)
--- 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 3e60f090079bdeb2b83b99097f3c99d6822260e4..435513d314a65195a9b80c447ec7e9933db22940 100644 (file)
--- 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)))