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.cdiff --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 <berra...@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
signature.asc
Description: PGP signature
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
