[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH] rpc: add testing of RPC JSON (de)serialization



On Fri, May 22, 2015 at 01:36:14PM +0100, Daniel P. Berrange wrote:
The virNetServer class has the ability to serialize its state
to a JSON file, and then re-load that data after an in-place
execve() call to re-connect to active file handles. This data
format is critical ABI that must have compatibility across
releases, so it should be tested...
---
src/libvirt_remote.syms                            |   1 +
src/rpc/virnetserver.c                             |   4 +-
src/rpc/virnetserver.h                             |   3 +
src/rpc/virnetserverclient.c                       |  13 +-
src/rpc/virnetserverservice.c                      |   6 +-
tests/Makefile.am                                  |   7 +
tests/virnetserverdata/README                      |  14 +
.../virnetserverdata/input-data-anon-clients.json  |  63 +++++
tests/virnetserverdata/input-data-initial.json     |  62 +++++
.../virnetserverdata/output-data-anon-clients.json |  63 +++++
tests/virnetserverdata/output-data-initial.json    |  63 +++++
tests/virnetservertest.c                           | 290 +++++++++++++++++++++
12 files changed, 579 insertions(+), 10 deletions(-)
create mode 100644 tests/virnetserverdata/README
create mode 100644 tests/virnetserverdata/input-data-anon-clients.json
create mode 100644 tests/virnetserverdata/input-data-initial.json
create mode 100644 tests/virnetserverdata/output-data-anon-clients.json
create mode 100644 tests/virnetserverdata/output-data-initial.json
create mode 100644 tests/virnetservertest.c

diff --git a/src/libvirt_remote.syms b/src/libvirt_remote.syms
index 950e56e..bdd68f6 100644
--- a/src/libvirt_remote.syms
+++ b/src/libvirt_remote.syms
@@ -77,6 +77,7 @@ xdr_virNetMessageError;


# rpc/virnetserver.h
+virNetServerAddClient;
virNetServerAddProgram;
virNetServerAddService;
virNetServerAddShutdownInhibition;
diff --git a/src/rpc/virnetserver.c b/src/rpc/virnetserver.c
index 42427dc..091a1dc 100644
--- a/src/rpc/virnetserver.c
+++ b/src/rpc/virnetserver.c
@@ -259,8 +259,8 @@ static int virNetServerDispatchNewMessage(virNetServerClientPtr client,
}


-static int virNetServerAddClient(virNetServerPtr srv,
-                                 virNetServerClientPtr client)
+int virNetServerAddClient(virNetServerPtr srv,
+                          virNetServerClientPtr client)
{
    virObjectLock(srv);

diff --git a/src/rpc/virnetserver.h b/src/rpc/virnetserver.h
index 8c5ae07..4b452be 100644
--- a/src/rpc/virnetserver.h
+++ b/src/rpc/virnetserver.h
@@ -79,6 +79,9 @@ int virNetServerAddService(virNetServerPtr srv,
                           virNetServerServicePtr svc,
                           const char *mdnsEntryName);

+int virNetServerAddClient(virNetServerPtr srv,
+                          virNetServerClientPtr client);
+
int virNetServerAddProgram(virNetServerPtr srv,
                           virNetServerProgramPtr prog);

diff --git a/src/rpc/virnetserverclient.c b/src/rpc/virnetserverclient.c
index 34c1994..0e3a71f 100644
--- a/src/rpc/virnetserverclient.c
+++ b/src/rpc/virnetserverclient.c
@@ -536,13 +536,14 @@ virJSONValuePtr virNetServerClientPreExecRestart(virNetServerClientPtr client)
        goto error;
    }

-    if (client->privateData && client->privateDataPreExecRestart &&
-        !(child = client->privateDataPreExecRestart(client, client->privateData)))
-        goto error;
+    if (client->privateData && client->privateDataPreExecRestart) {
+        if (!(child = client->privateDataPreExecRestart(client, client->privateData)))
+            goto error;

-    if (virJSONValueObjectAppend(object, "privateData", child) < 0) {
-        virJSONValueFree(child);
-        goto error;
+        if (virJSONValueObjectAppend(object, "privateData", child) < 0) {
+            virJSONValueFree(child);
+            goto error;
+        }
    }

    virObjectUnlock(client);
diff --git a/src/rpc/virnetserverservice.c b/src/rpc/virnetserverservice.c
index d3cf31a..4df26cb 100644
--- a/src/rpc/virnetserverservice.c
+++ b/src/rpc/virnetserverservice.c
@@ -303,12 +303,15 @@ virNetServerServicePtr virNetServerServiceNewFD(int fd,

        /* IO callback is initially disabled, until we're ready
         * to deal with incoming clients */
+        virObjectRef(svc);
        if (virNetSocketAddIOCallback(svc->socks[i],
                                      0,
                                      virNetServerServiceAccept,
                                      svc,
-                                      virObjectFreeCallback) < 0)
+                                      virObjectFreeCallback) < 0) {
+            virObjectUnref(svc);
            goto error;
+        }
    }


@@ -388,7 +391,6 @@ virNetServerServicePtr virNetServerServiceNewPostExecRestart(virJSONValuePtr obj
                                      svc,
                                      virObjectFreeCallback) < 0) {
            virObjectUnref(svc);
-            virObjectUnref(sock);
            goto error;
        }
    }
diff --git a/tests/Makefile.am b/tests/Makefile.am
index 8e2dbec..c9e2c8a 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -188,6 +188,7 @@ if WITH_REMOTE
test_programs += \
	virnetmessagetest \
	virnetsockettest \
+	virnetservertest \
	virnetserverclienttest \
	$(NULL)
if WITH_GNUTLS
@@ -921,6 +922,12 @@ virnetsockettest_SOURCES = \
	virnetsockettest.c testutils.h testutils.c
virnetsockettest_LDADD = $(LDADDS)

+virnetservertest_SOURCES = \
+	virnetservertest.c \
+	testutils.h testutils.c
+virnetservertest_CFLAGS = $(XDR_CFLAGS) $(AM_CFLAGS)
+virnetservertest_LDADD = $(LDADDS)
+
virnetserverclienttest_SOURCES = \
	virnetserverclienttest.c \
	testutils.h testutils.c
diff --git a/tests/virnetserverdata/README b/tests/virnetserverdata/README
new file mode 100644
index 0000000..d6d79d2
--- /dev/null
+++ b/tests/virnetserverdata/README
@@ -0,0 +1,14 @@
+   virnetservertest data files
+   ===========================
+
+The various input-data-*.json files are a record of all the historical
+formats that libvirt has been able to produce data for. Everytime a
+new field is added to the JSON output, a *new* input data file should
+be created. We must not add new fields to existing input-data files,
+nor must we ever re-structure them if code changes, as we must check
+new code handles the legacy formats.
+
+The various output-data-*.json files are the record of what the *new*
+JSON output should look like for the correspondingly named input-data
+file. It is permissible to change the existing output-data-*.json
+files if the format we save in is updated.

It might be worth saying that if new format is added, output-data can
be used as new input-data in order to check all possible combinations.
But that won't make sense until the server -> daemon rework is done,
so it might as well be me who adds it there.

diff --git a/tests/virnetserverdata/input-data-anon-clients.json b/tests/virnetserverdata/input-data-anon-clients.json
new file mode 100644
index 0000000..ed91b57
--- /dev/null
+++ b/tests/virnetserverdata/input-data-anon-clients.json
@@ -0,0 +1,63 @@
+{
+    "min_workers": 10,
+    "max_workers": 50,
+    "priority_workers": 5,
+    "max_clients": 100,
+    "max_anonymous_clients": 10,
+    "keepaliveInterval": 120,
+    "keepaliveCount": 5,
+    "keepaliveRequired": true,
+    "mdnsGroupName": "libvirtTest",
+    "services": [
+        {
+            "auth": 0,
+            "readonly": true,
+            "nrequests_client_max": 2,
+            "socks": [
+                {
+                    "fd": 100,
+                    "errfd": -1,
+                    "pid": 0,
+                    "isClient": false
+                }
+            ]
+        },
+        {
+            "auth": 2,
+            "readonly": false,
+            "nrequests_client_max": 5,
+            "socks": [
+                {
+                    "fd": 101,
+                    "errfd": -1,
+                    "pid": 0,
+                    "isClient": false
+                }
+            ]
+        }
+    ],
+    "clients": [
+        {
+            "auth": 1,
+            "readonly": true,
+            "nrequests_max": 15,
+            "sock": {
+                "fd": 102,
+                "errfd": -1,
+                "pid": -1,
+                "isClient": true
+            }
+        },
+        {
+            "auth": 2,
+            "readonly": true,
+            "nrequests_max": 66,
+            "sock": {
+                "fd": 103,
+                "errfd": -1,
+                "pid": -1,
+                "isClient": true
+            }
+        }
+    ]
+}
diff --git a/tests/virnetserverdata/input-data-initial.json b/tests/virnetserverdata/input-data-initial.json
new file mode 100644
index 0000000..7956225
--- /dev/null
+++ b/tests/virnetserverdata/input-data-initial.json
@@ -0,0 +1,62 @@
+{
+    "min_workers": 10,
+    "max_workers": 50,
+    "priority_workers": 5,
+    "max_clients": 100,
+    "keepaliveInterval": 120,
+    "keepaliveCount": 5,
+    "keepaliveRequired": true,
+    "mdnsGroupName": "libvirtTest",
+    "services": [
+        {
+            "auth": 0,
+            "readonly": true,
+            "nrequests_client_max": 2,
+            "socks": [
+                {
+                    "fd": 100,
+                    "errfd": -1,
+                    "pid": 0,
+                    "isClient": false
+                }
+            ]
+        },
+        {
+            "auth": 2,
+            "readonly": false,
+            "nrequests_client_max": 5,
+            "socks": [
+                {
+                    "fd": 101,
+                    "errfd": -1,
+                    "pid": 0,
+                    "isClient": false
+                }
+            ]
+        }
+    ],
+    "clients": [
+        {
+            "auth": 1,
+            "readonly": true,
+            "nrequests_max": 15,
+            "sock": {
+                "fd": 102,
+                "errfd": -1,
+                "pid": -1,
+                "isClient": true
+            }
+        },
+        {
+            "auth": 2,
+            "readonly": true,
+            "nrequests_max": 66,
+            "sock": {
+                "fd": 103,
+                "errfd": -1,
+                "pid": -1,
+                "isClient": true
+            }
+        }
+    ]
+}
diff --git a/tests/virnetserverdata/output-data-anon-clients.json b/tests/virnetserverdata/output-data-anon-clients.json
new file mode 100644
index 0000000..ed91b57
--- /dev/null
+++ b/tests/virnetserverdata/output-data-anon-clients.json
@@ -0,0 +1,63 @@
+{
+    "min_workers": 10,
+    "max_workers": 50,
+    "priority_workers": 5,
+    "max_clients": 100,
+    "max_anonymous_clients": 10,
+    "keepaliveInterval": 120,
+    "keepaliveCount": 5,
+    "keepaliveRequired": true,
+    "mdnsGroupName": "libvirtTest",
+    "services": [
+        {
+            "auth": 0,
+            "readonly": true,
+            "nrequests_client_max": 2,
+            "socks": [
+                {
+                    "fd": 100,
+                    "errfd": -1,
+                    "pid": 0,
+                    "isClient": false
+                }
+            ]
+        },
+        {
+            "auth": 2,
+            "readonly": false,
+            "nrequests_client_max": 5,
+            "socks": [
+                {
+                    "fd": 101,
+                    "errfd": -1,
+                    "pid": 0,
+                    "isClient": false
+                }
+            ]
+        }
+    ],
+    "clients": [
+        {
+            "auth": 1,
+            "readonly": true,
+            "nrequests_max": 15,
+            "sock": {
+                "fd": 102,
+                "errfd": -1,
+                "pid": -1,
+                "isClient": true
+            }
+        },
+        {
+            "auth": 2,
+            "readonly": true,
+            "nrequests_max": 66,
+            "sock": {
+                "fd": 103,
+                "errfd": -1,
+                "pid": -1,
+                "isClient": true
+            }
+        }
+    ]
+}
diff --git a/tests/virnetserverdata/output-data-initial.json b/tests/virnetserverdata/output-data-initial.json
new file mode 100644
index 0000000..da02aac
--- /dev/null
+++ b/tests/virnetserverdata/output-data-initial.json
@@ -0,0 +1,63 @@
+{
+    "min_workers": 10,
+    "max_workers": 50,
+    "priority_workers": 5,
+    "max_clients": 100,
+    "max_anonymous_clients": 100,
+    "keepaliveInterval": 120,
+    "keepaliveCount": 5,
+    "keepaliveRequired": true,
+    "mdnsGroupName": "libvirtTest",
+    "services": [
+        {
+            "auth": 0,
+            "readonly": true,
+            "nrequests_client_max": 2,
+            "socks": [
+                {
+                    "fd": 100,
+                    "errfd": -1,
+                    "pid": 0,
+                    "isClient": false
+                }
+            ]
+        },
+        {
+            "auth": 2,
+            "readonly": false,
+            "nrequests_client_max": 5,
+            "socks": [
+                {
+                    "fd": 101,
+                    "errfd": -1,
+                    "pid": 0,
+                    "isClient": false
+                }
+            ]
+        }
+    ],
+    "clients": [
+        {
+            "auth": 1,
+            "readonly": true,
+            "nrequests_max": 15,
+            "sock": {
+                "fd": 102,
+                "errfd": -1,
+                "pid": -1,
+                "isClient": true
+            }
+        },
+        {
+            "auth": 2,
+            "readonly": true,
+            "nrequests_max": 66,
+            "sock": {
+                "fd": 103,
+                "errfd": -1,
+                "pid": -1,
+                "isClient": true
+            }
+        }
+    ]
+}
diff --git a/tests/virnetservertest.c b/tests/virnetservertest.c
new file mode 100644
index 0000000..08431d7
--- /dev/null
+++ b/tests/virnetservertest.c
@@ -0,0 +1,290 @@
+/*
+ * Copyright (C) 2015 Red Hat, Inc.
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library.  If not, see
+ * <http://www.gnu.org/licenses/>.
+ *
+ * Author: Daniel P. Berrange <berrange redhat com>
+ */
+
+#include <config.h>
+
+#include "testutils.h"
+#include "virerror.h"
+#include "rpc/virnetserver.h"
+
+#define VIR_FROM_THIS VIR_FROM_RPC
+
+#ifdef HAVE_SOCKETPAIR
+static virNetServerPtr
+testCreateServer(const char *host, int family)
+{
+    virNetServerPtr srv = NULL;
+    virNetServerServicePtr svc1 = NULL, svc2 = NULL;
+    virNetServerClientPtr cln1 = NULL, cln2 = NULL;
+    virNetSocketPtr sk1 = NULL, sk2 = NULL;
+    int fdclient[2];
+
+    if (socketpair(PF_UNIX, SOCK_STREAM, 0, fdclient) < 0) {
+        virReportSystemError(errno, "%s",
+                             "Cannot create socket pair");
+        goto cleanup;
+    }
+
+    if (!(srv = virNetServerNew(10, 50, 5, 100, 10,
+                                120, 5, true,
+                                "libvirtTest",
+                                NULL,
+                                NULL,
+                                NULL,
+                                NULL)))
+        goto error;
+
+    if (!(svc1 = virNetServerServiceNewTCP(host,
+                                           NULL,
+                                           family,

Your patch must've been based on some experimental out-of-tree
patches, because we don't specify family anywhere.  This won't
compile.

+                                           VIR_NET_SERVER_SERVICE_AUTH_NONE,
+# ifdef WITH_GNUTLS
+                                           NULL,
+# endif
+                                           true,
+                                           5,
+                                           2)))
+        goto error;
+
+    if (!(svc2 = virNetServerServiceNewTCP(host,
+                                           NULL,
+                                           VIR_NET_SERVER_SERVICE_AUTH_POLKIT,
+                                           family,

Same here.

+# ifdef WITH_GNUTLS
+                                           NULL,
+# endif
+                                           false,
+                                           25,
+                                           5)))
+        goto error;
+
+    if (virNetServerAddService(srv, svc1, "libvirt-ro") < 0)
+        goto error;
+    if (virNetServerAddService(srv, svc2, "libvirt-ro") < 0)
+        goto error;
+
+    if (virNetSocketNewConnectSockFD(fdclient[0], &sk1) < 0)
+        goto error;
+    if (virNetSocketNewConnectSockFD(fdclient[1], &sk2) < 0)
+        goto error;
+
+    if (!(cln1 = virNetServerClientNew(sk1,
+                                       VIR_NET_SERVER_SERVICE_AUTH_SASL,
+                                       true,
+                                       15,
+# ifdef WITH_GNUTLS
+                                       NULL,
+# endif
+                                       NULL, NULL, NULL, NULL)))
+        goto error;
+
+    if (!(cln2 = virNetServerClientNew(sk2,
+                                       VIR_NET_SERVER_SERVICE_AUTH_POLKIT,
+                                       true,
+                                       66,
+# ifdef WITH_GNUTLS
+                                       NULL,
+# endif
+                                       NULL, NULL, NULL, NULL)))
+        goto error;
+
+    if (virNetServerAddClient(srv, cln1) < 0)
+        goto error;
+
+    if (virNetServerAddClient(srv, cln2) < 0)
+        goto error;
+
+ cleanup:
+    virObjectUnref(cln1);
+    virObjectUnref(cln2);
+    virObjectUnref(svc1);
+    virObjectUnref(svc2);
+    return srv;
+
+ error:
+    virObjectUnref(srv);
+    srv = NULL;
+    goto cleanup;
+}
+
+static char *testGenerateJSON(void)
+{
+    virNetServerPtr srv = NULL;
+    virJSONValuePtr json = NULL;
+    char *jsonstr = NULL;
+    bool has_ipv4, has_ipv6;
+
+    /* Our pre-saved JSON file is created so that each service
+     * only has one socket. If we let libvirt bind to IPv4 and
+     * IPv6 we might end up with two sockets, so force one or
+     * the other based on what's available on thehost
+     */
+    if (virNetSocketCheckProtocols(&has_ipv4,
+                                   &has_ipv6) < 0)

And this function doesn't exist anywhere in the tree.  Is this based
on some sent not yet reviewed series I missed?

+        return NULL;
+
+    if (!has_ipv4 && !has_ipv6)
+        return NULL;
+
+    if (!(srv = testCreateServer(
+              has_ipv4 ? "127.0.0.1" : "::1",
+              has_ipv4 ? AF_INET : AF_INET6)))
+        goto cleanup;
+
+    if (!(json = virNetServerPreExecRestart(srv)))
+        goto cleanup;
+
+    if (!(jsonstr = virJSONValueToString(json, true)))
+        goto cleanup;
+    fprintf(stderr, "%s\n", jsonstr);
+ cleanup:
+    virNetServerClose(srv);
+    virObjectUnref(srv);
+    virJSONValueFree(json);
+    return jsonstr;
+}
+
+
+struct testExecRestartData {
+    const char *jsonfile;
+};
+
+static int testExecRestart(const void *opaque)
+{
+    int ret = -1;
+    virNetServerPtr srv = NULL;
+    const struct testExecRestartData *data = opaque;
+    char *infile = NULL, *outfile = NULL;
+    char *injsonstr = NULL, *outjsonstr = NULL;
+    virJSONValuePtr injson = NULL, outjson = NULL;
+    int fdclient[2] = { -1, -1 }, fdserver[2] = { -1, -1 };
+
+    if (socketpair(PF_UNIX, SOCK_STREAM, 0, fdclient) < 0) {
+        virReportSystemError(errno, "%s",
+                             "Cannot create socket pair");
+        goto cleanup;
+    }
+
+    if (socketpair(PF_UNIX, SOCK_STREAM, 0, fdserver) < 0) {
+        virReportSystemError(errno, "%s",
+                             "Cannot create socket pair");
+        goto cleanup;
+    }
+
+    /* We're blindly assuming the test case isn't using
+     * fds 100->103 for something else, which is probably
+     * fairly reasonable in general
+     */
+    dup2(fdserver[0], 100);
+    dup2(fdserver[1], 101);
+    dup2(fdclient[0], 102);
+    dup2(fdclient[1], 103);
+
+    if (virAsprintf(&infile, "%s/virnetserverdata/input-data-%s.json",
+                    abs_srcdir, data->jsonfile) < 0)
+        goto cleanup;
+
+    if (virAsprintf(&outfile, "%s/virnetserverdata/output-data-%s.json",
+                    abs_srcdir, data->jsonfile) < 0)
+        goto cleanup;
+
+    if (virFileReadAll(infile, 8192, &injsonstr) < 0)
+        goto cleanup;
+
+    if (!(injson = virJSONValueFromString(injsonstr)))
+        goto cleanup;
+
+    if (!(srv = virNetServerNewPostExecRestart(injson,
+                                               NULL, NULL, NULL,
+                                               NULL, NULL)))
+        goto cleanup;
+
+    if (!(outjson = virNetServerPreExecRestart(srv)))
+        goto cleanup;
+
+    if (!(outjsonstr = virJSONValueToString(outjson, true)))
+        goto cleanup;
+
+    if (virtTestCompareToFile(outjsonstr, outfile) < 0)
+        goto fail;
+
+    ret = 0;
+
+ cleanup:
+    if (ret < 0) {
+        virErrorPtr err = virGetLastError();
+        fprintf(stderr, "%s\n", err->message);

By the way, if there is no error set here, it will sigsegv.  Like for me
right now because I compile --without-avahi and virNetServerMDNSNew()
does return NULL without setting an error.  Even though this is a
problem that needs to be resolved by itself (that no PostExec will work
right now --without-avahi if there is a groupName set for the server), I
think this could use the virErrorGenericFailure() that could be changed
so it rewrites the error only if err->code == VIR_ERR_OK (the condition
before its only usage).

Other than that it looks fine.

+    }
+ fail:
+    VIR_FREE(infile);
+    VIR_FREE(outfile);
+    VIR_FREE(injsonstr);
+    VIR_FREE(outjsonstr);
+    virJSONValueFree(injson);
+    virJSONValueFree(outjson);
+    virObjectUnref(srv);
+    VIR_FORCE_CLOSE(fdserver[0]);
+    VIR_FORCE_CLOSE(fdserver[1]);
+    VIR_FORCE_CLOSE(fdclient[0]);
+    VIR_FORCE_CLOSE(fdclient[1]);
+    return ret;
+}
+
+
+static int
+mymain(void)
+{
+    int ret = 0;
+
+    /* Hack to make it easier to generate new JSON files when
+     * the RPC classes change. Just set this env var, save
+     * the generated JSON, and replace the file descriptor
+     * numbers with 100, 101, 102, 103.
+     */
+    if (getenv("VIR_GENERATE_JSON")) {
+        char *json = testGenerateJSON();
+        fprintf(stdout, "%s\n", json);
+        VIR_FREE(json);
+        return EXIT_SUCCESS;
+    }
+
+# define EXEC_RESTART_TEST(file)                        \
+    do {                                                \
+        struct testExecRestartData data = {             \
+            file                                        \
+        };                                              \
+        if (virtTestRun("ExecRestart " file,            \
+                        testExecRestart, &data) < 0)    \
+            ret = -1;                                   \
+    } while (0)
+
+    EXEC_RESTART_TEST("initial");
+    EXEC_RESTART_TEST("anon-clients");
+
+    return ret == 0 ? EXIT_SUCCESS : EXIT_FAILURE;
+}
+#else
+static int
+mymain(void)
+{
+    return EXIT_AM_SKIP;
+}
+#endif
+VIRT_TEST_MAIN(mymain);
--
2.1.0

Attachment: signature.asc
Description: PGP signature


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]