[libvirt] [PATCH v2 0/5] RPC JSON (de)serialization and fixes

I took the liberty of changing Daniel's version and fix it up a bit. I've split it into multiple patches, removed unneeded functions, and fixed it for building without avahi. First version here: https://www.redhat.com/archives/libvir-list/2015-May/msg00812.html Daniel P. Berrange (4): rpc: add testing of RPC JSON (de)serialization rpc: Make virNetServerAddClient function dynamic rpc: Don't use unrelated value as privateData of client rpc: Fix reference counting around virNetSocketAddIOCallback Martin Kletzander (1): mdns: Set error when failing due to missing avahi src/libvirt_remote.syms | 1 + src/rpc/virnetserver.c | 4 +- src/rpc/virnetserver.h | 3 + src/rpc/virnetserverclient.c | 13 +- src/rpc/virnetservermdns.c | 8 +- src/rpc/virnetserverservice.c | 6 +- tests/Makefile.am | 7 + tests/virnetserverdata/README | 14 + .../virnetserverdata/input-data-anon-clients.json | 62 +++++ .../input-data-initial-nomdns.json | 61 +++++ tests/virnetserverdata/input-data-initial.json | 62 +++++ .../virnetserverdata/output-data-anon-clients.json | 62 +++++ .../output-data-initial-nomdns.json | 62 +++++ tests/virnetserverdata/output-data-initial.json | 63 +++++ tests/virnetservertest.c | 284 +++++++++++++++++++++ 15 files changed, 698 insertions(+), 14 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-nomdns.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-nomdns.json create mode 100644 tests/virnetserverdata/output-data-initial.json create mode 100644 tests/virnetservertest.c -- 2.4.2

When building without avahi support, we used VIR_DEBUG() to note that to the user. However, functions that fail because of that (return NULL/-1) did not set the error message. This was the only file that forgot to do such thing. Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/rpc/virnetservermdns.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/rpc/virnetservermdns.c b/src/rpc/virnetservermdns.c index 131968061c5f..7f12a2947363 100644 --- a/src/rpc/virnetservermdns.c +++ b/src/rpc/virnetservermdns.c @@ -617,14 +617,14 @@ static const char *unsupported = N_("avahi not available at build time"); virNetServerMDNS * virNetServerMDNSNew(void) { - VIR_DEBUG("%s", _(unsupported)); + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _(unsupported)); return NULL; } int virNetServerMDNSStart(virNetServerMDNS *mdns ATTRIBUTE_UNUSED) { - VIR_DEBUG("%s", _(unsupported)); + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _(unsupported)); return -1; } @@ -632,7 +632,7 @@ virNetServerMDNSGroupPtr virNetServerMDNSAddGroup(virNetServerMDNS *mdns ATTRIBUTE_UNUSED, const char *name ATTRIBUTE_UNUSED) { - VIR_DEBUG("%s", _(unsupported)); + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _(unsupported)); return NULL; } @@ -648,7 +648,7 @@ virNetServerMDNSAddEntry(virNetServerMDNSGroupPtr group ATTRIBUTE_UNUSED, const char *type ATTRIBUTE_UNUSED, int port ATTRIBUTE_UNUSED) { - VIR_DEBUG("%s", _(unsupported)); + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _(unsupported)); return NULL; } -- 2.4.2

On Thu, Jun 04, 2015 at 07:35:08PM +0200, Martin Kletzander wrote:
When building without avahi support, we used VIR_DEBUG() to note that to the user. However, functions that fail because of that (return NULL/-1) did not set the error message. This was the only file that forgot to do such thing.
Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/rpc/virnetservermdns.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
ACK Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|

From: "Daniel P. Berrange" <berrange@redhat.com> 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... Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- tests/Makefile.am | 7 + tests/virnetserverdata/README | 14 + .../virnetserverdata/input-data-anon-clients.json | 62 +++++ .../input-data-initial-nomdns.json | 61 +++++ tests/virnetserverdata/input-data-initial.json | 62 +++++ .../virnetserverdata/output-data-anon-clients.json | 62 +++++ .../output-data-initial-nomdns.json | 62 +++++ tests/virnetserverdata/output-data-initial.json | 63 +++++ tests/virnetservertest.c | 284 +++++++++++++++++++++ 9 files changed, 677 insertions(+) create mode 100644 tests/virnetserverdata/README create mode 100644 tests/virnetserverdata/input-data-anon-clients.json create mode 100644 tests/virnetserverdata/input-data-initial-nomdns.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-nomdns.json create mode 100644 tests/virnetserverdata/output-data-initial.json create mode 100644 tests/virnetservertest.c diff --git a/tests/Makefile.am b/tests/Makefile.am index 8e2dbec9af56..c9e2c8a0afa2 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 000000000000..d6d79d27d5ea --- /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. diff --git a/tests/virnetserverdata/input-data-anon-clients.json b/tests/virnetserverdata/input-data-anon-clients.json new file mode 100644 index 000000000000..8a51ff53d6cf --- /dev/null +++ b/tests/virnetserverdata/input-data-anon-clients.json @@ -0,0 +1,62 @@ +{ + "min_workers": 10, + "max_workers": 50, + "priority_workers": 5, + "max_clients": 100, + "max_anonymous_clients": 10, + "keepaliveInterval": 120, + "keepaliveCount": 5, + "keepaliveRequired": true, + "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-nomdns.json b/tests/virnetserverdata/input-data-initial-nomdns.json new file mode 100644 index 000000000000..02bb42748774 --- /dev/null +++ b/tests/virnetserverdata/input-data-initial-nomdns.json @@ -0,0 +1,61 @@ +{ + "min_workers": 10, + "max_workers": 50, + "priority_workers": 5, + "max_clients": 100, + "keepaliveInterval": 120, + "keepaliveCount": 5, + "keepaliveRequired": true, + "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 000000000000..795622535b88 --- /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 000000000000..8a51ff53d6cf --- /dev/null +++ b/tests/virnetserverdata/output-data-anon-clients.json @@ -0,0 +1,62 @@ +{ + "min_workers": 10, + "max_workers": 50, + "priority_workers": 5, + "max_clients": 100, + "max_anonymous_clients": 10, + "keepaliveInterval": 120, + "keepaliveCount": 5, + "keepaliveRequired": true, + "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-nomdns.json b/tests/virnetserverdata/output-data-initial-nomdns.json new file mode 100644 index 000000000000..8d38297af63b --- /dev/null +++ b/tests/virnetserverdata/output-data-initial-nomdns.json @@ -0,0 +1,62 @@ +{ + "min_workers": 10, + "max_workers": 50, + "priority_workers": 5, + "max_clients": 100, + "max_anonymous_clients": 100, + "keepaliveInterval": 120, + "keepaliveCount": 5, + "keepaliveRequired": true, + "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 000000000000..da02aac0aa9f --- /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 000000000000..1285a129ecf6 --- /dev/null +++ b/tests/virnetservertest.c @@ -0,0 +1,284 @@ +/* + * 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(void) +{ + 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, +# ifdef WITH_AVAHI + "libvirtTest", +# else + NULL, +# endif + NULL, + NULL, + NULL, + NULL))) + goto error; + + if (!(svc1 = virNetServerServiceNewTCP(NULL, + NULL, + VIR_NET_SERVER_SERVICE_AUTH_NONE, +# ifdef WITH_GNUTLS + NULL, +# endif + true, + 5, + 2))) + goto error; + + if (!(svc2 = virNetServerServiceNewTCP(NULL, + NULL, + VIR_NET_SERVER_SERVICE_AUTH_POLKIT, +# 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; + + if (!(srv = testCreateServer())) + 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(); + /* Rather be safe, we have lot of missing errors */ + if (err) + fprintf(stderr, "%s\n", err->message); + else + fprintf(stderr, "%s\n", "Unknown error"); + } + 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) + +# ifdef WITH_AVAHI + EXEC_RESTART_TEST("initial"); +# endif + EXEC_RESTART_TEST("initial-nomdns"); + 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.4.2

On Thu, Jun 04, 2015 at 07:35:09PM +0200, Martin Kletzander wrote:
From: "Daniel P. Berrange" <berrange@redhat.com>
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...
Signed-off-by: Martin Kletzander <mkletzan@redhat.com>
Sorry in my previous posting of this, I messed up and did not send the two pre-requisite patches for allowing us to restrict the address family usage, which is important for testCreateServer to produce accurate results in dual ipv4/6 systems. I see you have deleted that bit of code, but we still need it, so let me just send the original patches again, and then we can just re-add the bits to this version of yours. Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|

On Fri, Jun 05, 2015 at 09:46:24AM +0100, Daniel P. Berrange wrote:
On Thu, Jun 04, 2015 at 07:35:09PM +0200, Martin Kletzander wrote:
From: "Daniel P. Berrange" <berrange@redhat.com>
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...
Signed-off-by: Martin Kletzander <mkletzan@redhat.com>
Sorry in my previous posting of this, I messed up and did not send the two pre-requisite patches for allowing us to restrict the address family usage, which is important for testCreateServer to produce accurate results in dual ipv4/6 systems. I see you have deleted that bit of code, but we still need it, so let me just send the original patches again, and then we can just re-add the bits to this version of yours.
I was wondering why you've replied with this on patch 2/5 and now I see I forgot to reorder them. I'll push those other (ACKed) clean-ups in a while. About the address family usage, I couldn't find out what could the problem be if the test would be ran on such hosts. However, I believe you're right, so go ahead. I have changed only a minimum of your code, so just send your version when it's ready. In the meantime I'll use ththis version to rebase the Admin API series upon, so I can test that it re-execs properly. I would really like for that to go in this release.
Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|

From: "Daniel P. Berrange" <berrange@redhat.com> As opposed to 'static'; by exporting it (privately). Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/libvirt_remote.syms | 1 + src/rpc/virnetserver.c | 4 ++-- src/rpc/virnetserver.h | 3 +++ 3 files changed, 6 insertions(+), 2 deletions(-) diff --git a/src/libvirt_remote.syms b/src/libvirt_remote.syms index 6b520b5fa923..0d650b6b2737 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 42427dc5babb..091a1dc0bc8f 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 8c5ae072db28..4b452be5a1ad 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); -- 2.4.2

On Thu, Jun 04, 2015 at 07:35:10PM +0200, Martin Kletzander wrote:
From: "Daniel P. Berrange" <berrange@redhat.com>
As opposed to 'static'; by exporting it (privately).
Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/libvirt_remote.syms | 1 + src/rpc/virnetserver.c | 4 ++-- src/rpc/virnetserver.h | 3 +++ 3 files changed, 6 insertions(+), 2 deletions(-)
ACK Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|

From: "Daniel P. Berrange" <berrange@redhat.com> Append privateData of the client only if there are any, otherwise the previous value (socket data) will get there again. Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/rpc/virnetserverclient.c | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/src/rpc/virnetserverclient.c b/src/rpc/virnetserverclient.c index 34c199445401..0e3a71f9b271 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); -- 2.4.2

On Thu, Jun 04, 2015 at 07:35:11PM +0200, Martin Kletzander wrote:
From: "Daniel P. Berrange" <berrange@redhat.com>
Append privateData of the client only if there are any, otherwise the previous value (socket data) will get there again.
Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/rpc/virnetserverclient.c | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-)
ACK Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|

From: "Daniel P. Berrange" <berrange@redhat.com> Ref service passed as a parameter to the callback. And don't unref the socket that is part of the service being passed at another point in code. Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/rpc/virnetserverservice.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/rpc/virnetserverservice.c b/src/rpc/virnetserverservice.c index d84b6defd4e5..9087473efd39 100644 --- a/src/rpc/virnetserverservice.c +++ b/src/rpc/virnetserverservice.c @@ -301,12 +301,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; + } } @@ -386,7 +389,6 @@ virNetServerServicePtr virNetServerServiceNewPostExecRestart(virJSONValuePtr obj svc, virObjectFreeCallback) < 0) { virObjectUnref(svc); - virObjectUnref(sock); goto error; } } -- 2.4.2

On Thu, Jun 04, 2015 at 07:35:12PM +0200, Martin Kletzander wrote:
From: "Daniel P. Berrange" <berrange@redhat.com>
Ref service passed as a parameter to the callback. And don't unref the socket that is part of the service being passed at another point in code.
Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/rpc/virnetserverservice.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)
ACK Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
participants (2)
-
Daniel P. Berrange
-
Martin Kletzander