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>

src/libvirt_remote.syms

@@ -61,7 +61,6 @@ virNetClientStreamSetError;
 
 # rpc/virnetdaemon.h
 virNetDaemonAddServer;
-virNetDaemonAddServerPostExec;
 virNetDaemonAddShutdownInhibition;
 virNetDaemonAddSignalHandler;
 virNetDaemonAutoShutdown;

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,19 +290,13 @@ virLockDaemonNewPostExecRestart(virJSONValuePtr object, bool privileged)
         }
     }
 
-    if (!(lockd->dmn = virNetDaemonNewPostExecRestart(child)))
+    if (!(lockd->dmn = virNetDaemonNewPostExecRestart(child,
+                                                      ARRAY_CARDINALITY(serverNames),
+                                                      serverNames,
+                                                      virLockDaemonNewServerPostExecRestart,
+                                                      (void*)(intptr_t)(privileged ? 0x1 : 0x0))))
         goto error;
 
-    if (!(srv = virNetDaemonAddServerPostExec(lockd->dmn,
-                                              "virtlockd",
-                                              virLockDaemonClientNew,
-                                              virLockDaemonClientNewPostExecRestart,
-                                              virLockDaemonClientPreExecRestart,
-                                              virLockDaemonClientFree,
-                                              (void*)(intptr_t)(privileged ? 0x1 : 0x0))))
-        goto error;
-    virObjectUnref(srv);
-
     return lockd;
 
  error:

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,19 +235,13 @@ virLogDaemonNewPostExecRestart(virJSONValuePtr object, bool privileged,
         goto error;
     }
 
-    if (!(logd->dmn = virNetDaemonNewPostExecRestart(child)))
+    if (!(logd->dmn = virNetDaemonNewPostExecRestart(child,
+                                                     ARRAY_CARDINALITY(serverNames),
+                                                     serverNames,
+                                                     virLogDaemonNewServerPostExecRestart,
+                                                     (void*)(intptr_t)(privileged ? 0x1 : 0x0))))
         goto error;
 
-    if (!(srv = virNetDaemonAddServerPostExec(logd->dmn,
-                                              "virtlogd",
-                                              virLogDaemonClientNew,
-                                              virLogDaemonClientNewPostExecRestart,
-                                              virLogDaemonClientPreExecRestart,
-                                              virLogDaemonClientFree,
-                                              (void*)(intptr_t)(privileged ? 0x1 : 0x0))))
-        goto error;
-    virObjectUnref(srv);
-
     if (!(child = virJSONValueObjectGet(object, "handler"))) {
         virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
                        _("Malformed daemon data from JSON file"));

src/rpc/virnetdaemon.c

@@ -262,85 +262,38 @@ virNetDaemonGetServers(virNetDaemonPtr dmn,
 }
 
 
-virNetServerPtr
+struct virNetDaemonServerData {
-virNetDaemonAddServerPostExec(virNetDaemonPtr dmn,
+    virNetDaemonPtr dmn;
-                              const char *serverName,
+    virNetDaemonNewServerPostExecRestart cb;
-                              virNetServerClientPrivNew clientPrivNew,
+    void *opaque;
-                              virNetServerClientPrivNewPostExecRestart clientPrivNewPostExecRestart,
+};
-                              virNetServerClientPrivPreExecRestart clientPrivPreExecRestart,
+
-                              virFreeCallback clientPrivFree,
+static int
-                              void *clientPrivOpaque)
+virNetDaemonServerIterator(const char *key,
+                           virJSONValuePtr value,
+                           void *opaque)
 {
-    virJSONValuePtr object = NULL;
+    struct virNetDaemonServerData *data = opaque;
-    virNetServerPtr srv = NULL;
+    virNetServerPtr srv;
-
-    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;
-        }
-    }
-
-    srv = virNetServerNewPostExecRestart(object,
-                                         serverName,
-                                         clientPrivNew,
-                                         clientPrivNewPostExecRestart,
-                                         clientPrivPreExecRestart,
-                                         clientPrivFree,
-                                         clientPrivOpaque);
 
+    VIR_DEBUG("Creating server '%s'", key);
+    srv = data->cb(data->dmn, key, value, data->opaque);
     if (!srv)
-        goto error;
+        return -1;
 
-    if (virHashAddEntry(dmn->servers, serverName, srv) < 0)
+    if (virHashAddEntry(data->dmn->servers, key, srv) < 0)
-        goto error;
+        return -1;
-    virObjectRef(srv);
 
-    virJSONValueFree(object);
+    return 0;
-    virObjectUnlock(dmn);
-    return srv;
-
- error:
-    virObjectUnlock(dmn);
-    virObjectUnref(srv);
-    virJSONValueFree(object);
-    return NULL;
 }
 
 
 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)))
+    if (!new_version) {
-        goto error;
+        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;

src/rpc/virnetdaemon.h

@@ -40,15 +40,15 @@ virNetDaemonPtr virNetDaemonNew(void);
 int virNetDaemonAddServer(virNetDaemonPtr dmn,
                           virNetServerPtr srv);
 
-virNetServerPtr virNetDaemonAddServerPostExec(virNetDaemonPtr dmn,
+typedef virNetServerPtr (*virNetDaemonNewServerPostExecRestart)(virNetDaemonPtr dmn,
-                                              const char *serverName,
+                                                                const char *name,
-                                              virNetServerClientPrivNew clientPrivNew,
+                                                                virJSONValuePtr object,
-                                              virNetServerClientPrivNewPostExecRestart clientPrivNewPostExecRestart,
+                                                                void *opaque);
-                                              virNetServerClientPrivPreExecRestart clientPrivPreExecRestart,
+virNetDaemonPtr virNetDaemonNewPostExecRestart(virJSONValuePtr object,
-                                              virFreeCallback clientPrivFree,
+                                               size_t nDefServerNames,
-                                              void *clientPrivOpaque);
+                                               const char **defServerNames,
-
+                                               virNetDaemonNewServerPostExecRestart cb,
-virNetDaemonPtr virNetDaemonNewPostExecRestart(virJSONValuePtr object);
+                                               void *opaque);
 
 virJSONValuePtr virNetDaemonPreExecRestart(virNetDaemonPtr dmn);
 

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],
+        if (!virNetDaemonHasServer(dmn, data->serverNames[i])) {
-                                                  NULL, NULL, NULL,
+            virReportError(VIR_ERR_INTERNAL_ERROR,
-                                                  NULL, NULL)))
+                           "Server %s was not created",
+                           data->serverNames[i]);
             goto cleanup;
-        srv = NULL;
+        }
     }
 
     if (!(outjson = virNetDaemonPreExecRestart(dmn)))