Since its introduction in 2011 (particularly in commit f4324e329275), the option doesn't work. It just effectively disables all incoming connections. That's because the client private data that contain the 'keepalive_supported' boolean, are initialized to zeroes so the bool is false and the only other place where the bool is used is when checking whether the client supports keepalive. Thus, according to the server, no client supports keepalive. Removing this instead of fixing it is better because a) apparently nobody ever tried it since 2011 (4 years without one month) and b) we cannot know whether the client supports keepalive until we get a ping or pong keepalive packet. And that won't happen untile after we dispatched the ConnectOpen call. Another two reasons would be c) the keepalive_required was tracked on the server level, but keepalive_supported was in private data of the client as well as the check that was made in the remote layer, thus making all other instances of virNetServer miss this feature unless they all implemented it for themselves and d) we can always add it back in case there is a request and a use-case for it. Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- v3: - Kept the config option in place the same way we did with log_buffer_size - Moved the deprecated options together v2: - Added a new daemontest file with current input/output. daemon/libvirtd-config.c | 4 - daemon/libvirtd-config.h | 2 - daemon/libvirtd.c | 2 - daemon/libvirtd.conf | 9 +- daemon/libvirtd.h | 1 - daemon/remote.c | 8 +- daemon/test_libvirtd.aug.in | 2 +- src/libvirt_remote.syms | 1 - src/locking/lock_daemon.c | 2 +- src/lxc/lxc_controller.c | 2 +- src/rpc/virnetserver.c | 25 +---- src/rpc/virnetserver.h | 3 - tests/libvirtdconftest.c | 4 +- .../input-data-no-keepalive-required.json | 124 +++++++++++++++++++++ .../virnetdaemondata/output-data-admin-nomdns.json | 2 - .../virnetdaemondata/output-data-anon-clients.json | 1 - .../output-data-initial-nomdns.json | 1 - tests/virnetdaemondata/output-data-initial.json | 1 - .../output-data-no-keepalive-required.json | 124 +++++++++++++++++++++ tests/virnetdaemontest.c | 2 +- 20 files changed, 262 insertions(+), 58 deletions(-) create mode 100644 tests/virnetdaemondata/input-data-no-keepalive-required.json create mode 100644 tests/virnetdaemondata/output-data-no-keepalive-required.json diff --git a/daemon/libvirtd-config.c b/daemon/libvirtd-config.c index 10dcc423d2db..c31c8b2e9ab5 100644 --- a/daemon/libvirtd-config.c +++ b/daemon/libvirtd-config.c @@ -292,7 +292,6 @@ daemonConfigNew(bool privileged ATTRIBUTE_UNUSED) data->keepalive_interval = 5; data->keepalive_count = 5; - data->keepalive_required = 0; data->admin_min_workers = 5; data->admin_max_workers = 20; @@ -302,7 +301,6 @@ daemonConfigNew(bool privileged ATTRIBUTE_UNUSED) data->admin_keepalive_interval = 5; data->admin_keepalive_count = 5; - data->admin_keepalive_required = 0; localhost = virGetHostname(); if (localhost == NULL) { @@ -471,11 +469,9 @@ daemonConfigLoadOptions(struct daemonConfig *data, GET_CONF_INT(conf, filename, keepalive_interval); GET_CONF_UINT(conf, filename, keepalive_count); - GET_CONF_UINT(conf, filename, keepalive_required); GET_CONF_INT(conf, filename, admin_keepalive_interval); GET_CONF_UINT(conf, filename, admin_keepalive_count); - GET_CONF_UINT(conf, filename, admin_keepalive_required); return 0; diff --git a/daemon/libvirtd-config.h b/daemon/libvirtd-config.h index 9cdae1a0cb59..3e1971d67f05 100644 --- a/daemon/libvirtd-config.h +++ b/daemon/libvirtd-config.h @@ -81,7 +81,6 @@ struct daemonConfig { int keepalive_interval; unsigned int keepalive_count; - int keepalive_required; int admin_min_workers; int admin_max_workers; @@ -91,7 +90,6 @@ struct daemonConfig { int admin_keepalive_interval; unsigned int admin_keepalive_count; - int admin_keepalive_required; }; diff --git a/daemon/libvirtd.c b/daemon/libvirtd.c index 71db4a042c7f..250094bd21dd 100644 --- a/daemon/libvirtd.c +++ b/daemon/libvirtd.c @@ -1389,7 +1389,6 @@ int main(int argc, char **argv) { config->max_anonymous_clients, config->keepalive_interval, config->keepalive_count, - !!config->keepalive_required, config->mdns_adv ? config->mdns_name : NULL, remoteClientInitHook, NULL, @@ -1464,7 +1463,6 @@ int main(int argc, char **argv) { 0, config->admin_keepalive_interval, config->admin_keepalive_count, - !!config->admin_keepalive_required, NULL, remoteAdmClientInitHook, NULL, diff --git a/daemon/libvirtd.conf b/daemon/libvirtd.conf index ac06cdd79103..514e6e4813bd 100644 --- a/daemon/libvirtd.conf +++ b/daemon/libvirtd.conf @@ -440,14 +440,15 @@ # #keepalive_interval = 5 #keepalive_count = 5 + # -# If set to 1, libvirtd will refuse to talk to clients that do not -# support keepalive protocol. Defaults to 0. +# These configuration options are no longer used. There is no way to +# restrict such clients from connecting since they first need to +# connect in order to ask for keepalive. # #keepalive_required = 1 +#admin_keepalive_required = 1 # Keepalive settings for the admin interface #admin_keepalive_interval = 5 #admin_keepalive_count = 5 -# -#admin_keepalive_required = 1 diff --git a/daemon/libvirtd.h b/daemon/libvirtd.h index 8c1a904893ab..efd4823ae18e 100644 --- a/daemon/libvirtd.h +++ b/daemon/libvirtd.h @@ -72,7 +72,6 @@ struct daemonClientPrivate { virConnectPtr conn; daemonClientStreamPtr streams; - bool keepalive_supported; }; /* Separate private data for admin connection */ diff --git a/daemon/remote.c b/daemon/remote.c index e9e2dcae80e0..3a3eb0913088 100644 --- a/daemon/remote.c +++ b/daemon/remote.c @@ -1290,7 +1290,7 @@ void *remoteClientInitHook(virNetServerClientPtr client, /*----- Functions. -----*/ static int -remoteDispatchConnectOpen(virNetServerPtr server, +remoteDispatchConnectOpen(virNetServerPtr server ATTRIBUTE_UNUSED, virNetServerClientPtr client, virNetMessagePtr msg ATTRIBUTE_UNUSED, virNetMessageErrorPtr rerr, @@ -1309,12 +1309,6 @@ remoteDispatchConnectOpen(virNetServerPtr server, goto cleanup; } - if (virNetServerKeepAliveRequired(server) && !priv->keepalive_supported) { - virReportError(VIR_ERR_OPERATION_FAILED, "%s", - _("keepalive support is required to connect")); - goto cleanup; - } - name = args->name ? *args->name : NULL; /* If this connection arrived on a readonly socket, force diff --git a/daemon/test_libvirtd.aug.in b/daemon/test_libvirtd.aug.in index 4921cbfb86b3..b0cb7eb14920 100644 --- a/daemon/test_libvirtd.aug.in +++ b/daemon/test_libvirtd.aug.in @@ -58,6 +58,6 @@ module Test_libvirtd = { "keepalive_interval" = "5" } { "keepalive_count" = "5" } { "keepalive_required" = "1" } + { "admin_keepalive_required" = "1" } { "admin_keepalive_interval" = "5" } { "admin_keepalive_count" = "5" } - { "admin_keepalive_required" = "1" } diff --git a/src/libvirt_remote.syms b/src/libvirt_remote.syms index 6bfdcfa819bf..90a453c8be9c 100644 --- a/src/libvirt_remote.syms +++ b/src/libvirt_remote.syms @@ -101,7 +101,6 @@ virNetServerAddProgram; virNetServerAddService; virNetServerClose; virNetServerHasClients; -virNetServerKeepAliveRequired; virNetServerNew; virNetServerNewPostExecRestart; virNetServerPreExecRestart; diff --git a/src/locking/lock_daemon.c b/src/locking/lock_daemon.c index ecbe03a4c154..c03502459acd 100644 --- a/src/locking/lock_daemon.c +++ b/src/locking/lock_daemon.c @@ -151,7 +151,7 @@ virLockDaemonNew(virLockDaemonConfigPtr config, bool privileged) if (!(lockd->srv = virNetServerNew(1, 1, 0, config->max_clients, config->max_clients, -1, 0, - false, NULL, + NULL, virLockDaemonClientNew, virLockDaemonClientPreExecRestart, virLockDaemonClientFree, diff --git a/src/lxc/lxc_controller.c b/src/lxc/lxc_controller.c index 110a55662be0..48a3597ed274 100644 --- a/src/lxc/lxc_controller.c +++ b/src/lxc/lxc_controller.c @@ -925,7 +925,7 @@ static int virLXCControllerSetupServer(virLXCControllerPtr ctrl) return -1; if (!(srv = virNetServerNew(0, 0, 0, 1, - 0, -1, 0, false, + 0, -1, 0, NULL, virLXCControllerClientPrivateNew, NULL, diff --git a/src/rpc/virnetserver.c b/src/rpc/virnetserver.c index 60a9714f6096..80b5588bf3c9 100644 --- a/src/rpc/virnetserver.c +++ b/src/rpc/virnetserver.c @@ -69,7 +69,6 @@ struct _virNetServer { int keepaliveInterval; unsigned int keepaliveCount; - bool keepaliveRequired; #ifdef WITH_GNUTLS virNetTLSContextPtr tls; @@ -312,7 +311,6 @@ virNetServerPtr virNetServerNew(size_t min_workers, size_t max_anonymous_clients, int keepaliveInterval, unsigned int keepaliveCount, - bool keepaliveRequired, const char *mdnsGroupName, virNetServerClientPrivNew clientPrivNew, virNetServerClientPrivPreExecRestart clientPrivPreExecRestart, @@ -338,7 +336,6 @@ virNetServerPtr virNetServerNew(size_t min_workers, srv->nclients_unauth_max = max_anonymous_clients; srv->keepaliveInterval = keepaliveInterval; srv->keepaliveCount = keepaliveCount; - srv->keepaliveRequired = keepaliveRequired; srv->clientPrivNew = clientPrivNew; srv->clientPrivPreExecRestart = clientPrivPreExecRestart; srv->clientPrivFree = clientPrivFree; @@ -380,7 +377,6 @@ virNetServerPtr virNetServerNewPostExecRestart(virJSONValuePtr object, unsigned int max_anonymous_clients; unsigned int keepaliveInterval; unsigned int keepaliveCount; - bool keepaliveRequired; const char *mdnsGroupName = NULL; if (virJSONValueObjectGetNumberUint(object, "min_workers", &min_workers) < 0) { @@ -423,11 +419,6 @@ virNetServerPtr virNetServerNewPostExecRestart(virJSONValuePtr object, _("Missing keepaliveCount data in JSON document")); goto error; } - if (virJSONValueObjectGetBoolean(object, "keepaliveRequired", &keepaliveRequired) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("Missing keepaliveRequired data in JSON document")); - goto error; - } if (virJSONValueObjectHasKey(object, "mdnsGroupName") && (!(mdnsGroupName = virJSONValueObjectGetString(object, "mdnsGroupName")))) { @@ -440,7 +431,7 @@ virNetServerPtr virNetServerNewPostExecRestart(virJSONValuePtr object, priority_workers, max_clients, max_anonymous_clients, keepaliveInterval, keepaliveCount, - keepaliveRequired, mdnsGroupName, + mdnsGroupName, clientPrivNew, clientPrivPreExecRestart, clientPrivFree, clientPrivOpaque))) goto error; @@ -573,11 +564,6 @@ virJSONValuePtr virNetServerPreExecRestart(virNetServerPtr srv) _("Cannot set keepaliveCount data in JSON document")); goto error; } - if (virJSONValueObjectAppendBoolean(object, "keepaliveRequired", srv->keepaliveRequired) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("Cannot set keepaliveRequired data in JSON document")); - goto error; - } if (srv->mdnsGroupName && virJSONValueObjectAppendString(object, "mdnsGroupName", srv->mdnsGroupName) < 0) { @@ -786,15 +772,6 @@ void virNetServerClose(virNetServerPtr srv) virObjectUnlock(srv); } -bool virNetServerKeepAliveRequired(virNetServerPtr srv) -{ - bool required; - virObjectLock(srv); - required = srv->keepaliveRequired; - virObjectUnlock(srv); - return required; -} - static inline size_t virNetServerTrackPendingAuthLocked(virNetServerPtr srv) { diff --git a/src/rpc/virnetserver.h b/src/rpc/virnetserver.h index 0e16e8fb1bf0..89d8db9b9ee4 100644 --- a/src/rpc/virnetserver.h +++ b/src/rpc/virnetserver.h @@ -41,7 +41,6 @@ virNetServerPtr virNetServerNew(size_t min_workers, size_t max_anonymous_clients, int keepaliveInterval, unsigned int keepaliveCount, - bool keepaliveRequired, const char *mdnsGroupName, virNetServerClientPrivNew clientPrivNew, virNetServerClientPrivPreExecRestart clientPrivPreExecRestart, @@ -74,8 +73,6 @@ int virNetServerSetTLSContext(virNetServerPtr srv, virNetTLSContextPtr tls); # endif -bool virNetServerKeepAliveRequired(virNetServerPtr srv); - size_t virNetServerTrackPendingAuth(virNetServerPtr srv); size_t virNetServerTrackCompletedAuth(virNetServerPtr srv); diff --git a/tests/libvirtdconftest.c b/tests/libvirtdconftest.c index d589d51cef30..61d861db1d1c 100644 --- a/tests/libvirtdconftest.c +++ b/tests/libvirtdconftest.c @@ -227,7 +227,9 @@ mymain(void) for (i = 0; params[i] != 0; i++) { const struct testCorruptData data = { params, filedata, filename, i }; /* Skip now ignored config param */ - if (STRPREFIX(filedata + params[i], "log_buffer_size")) + if (STRPREFIX(filedata + params[i], "log_buffer_size") || + STRPREFIX(filedata + params[i], "keepalive_required") || + STRPREFIX(filedata + params[i], "admin_keepalive_required")) continue; if (virtTestRun("Test corruption", testCorrupt, &data) < 0) ret = -1; diff --git a/tests/virnetdaemondata/input-data-no-keepalive-required.json b/tests/virnetdaemondata/input-data-no-keepalive-required.json new file mode 100644 index 000000000000..b5e4dc8e0391 --- /dev/null +++ b/tests/virnetdaemondata/input-data-no-keepalive-required.json @@ -0,0 +1,124 @@ +{ + "servers": [ + { + "min_workers": 10, + "max_workers": 50, + "priority_workers": 5, + "max_clients": 100, + "keepaliveInterval": 120, + "keepaliveCount": 5, + "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 + } + } + ] + }, + { + "min_workers": 2, + "max_workers": 50, + "priority_workers": 5, + "max_clients": 100, + "keepaliveInterval": 120, + "keepaliveCount": 5, + "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/virnetdaemondata/output-data-admin-nomdns.json b/tests/virnetdaemondata/output-data-admin-nomdns.json index 5df71a0d88c8..a814aeb80614 100644 --- a/tests/virnetdaemondata/output-data-admin-nomdns.json +++ b/tests/virnetdaemondata/output-data-admin-nomdns.json @@ -8,7 +8,6 @@ "max_anonymous_clients": 100, "keepaliveInterval": 120, "keepaliveCount": 5, - "keepaliveRequired": true, "services": [ { "auth": 0, @@ -70,7 +69,6 @@ "max_anonymous_clients": 100, "keepaliveInterval": 120, "keepaliveCount": 5, - "keepaliveRequired": true, "services": [ { "auth": 0, diff --git a/tests/virnetdaemondata/output-data-anon-clients.json b/tests/virnetdaemondata/output-data-anon-clients.json index 4e4332691aa7..05fc0ae00d3f 100644 --- a/tests/virnetdaemondata/output-data-anon-clients.json +++ b/tests/virnetdaemondata/output-data-anon-clients.json @@ -8,7 +8,6 @@ "max_anonymous_clients": 10, "keepaliveInterval": 120, "keepaliveCount": 5, - "keepaliveRequired": true, "services": [ { "auth": 0, diff --git a/tests/virnetdaemondata/output-data-initial-nomdns.json b/tests/virnetdaemondata/output-data-initial-nomdns.json index bef54bf94ad5..400e47bc9463 100644 --- a/tests/virnetdaemondata/output-data-initial-nomdns.json +++ b/tests/virnetdaemondata/output-data-initial-nomdns.json @@ -8,7 +8,6 @@ "max_anonymous_clients": 100, "keepaliveInterval": 120, "keepaliveCount": 5, - "keepaliveRequired": true, "services": [ { "auth": 0, diff --git a/tests/virnetdaemondata/output-data-initial.json b/tests/virnetdaemondata/output-data-initial.json index 9afa791d91fc..e875cffe5c01 100644 --- a/tests/virnetdaemondata/output-data-initial.json +++ b/tests/virnetdaemondata/output-data-initial.json @@ -8,7 +8,6 @@ "max_anonymous_clients": 100, "keepaliveInterval": 120, "keepaliveCount": 5, - "keepaliveRequired": true, "mdnsGroupName": "libvirtTest", "services": [ { diff --git a/tests/virnetdaemondata/output-data-no-keepalive-required.json b/tests/virnetdaemondata/output-data-no-keepalive-required.json new file mode 100644 index 000000000000..b5e4dc8e0391 --- /dev/null +++ b/tests/virnetdaemondata/output-data-no-keepalive-required.json @@ -0,0 +1,124 @@ +{ + "servers": [ + { + "min_workers": 10, + "max_workers": 50, + "priority_workers": 5, + "max_clients": 100, + "keepaliveInterval": 120, + "keepaliveCount": 5, + "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 + } + } + ] + }, + { + "min_workers": 2, + "max_workers": 50, + "priority_workers": 5, + "max_clients": 100, + "keepaliveInterval": 120, + "keepaliveCount": 5, + "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/virnetdaemontest.c b/tests/virnetdaemontest.c index ef45018f5873..fb8a6c0c0ec5 100644 --- a/tests/virnetdaemontest.c +++ b/tests/virnetdaemontest.c @@ -50,7 +50,7 @@ testCreateServer(const char *host, int family) } if (!(srv = virNetServerNew(10, 50, 5, 100, 10, - 120, 5, true, + 120, 5, mdns_group, NULL, NULL, -- 2.5.0