Re: [libvirt] [PATCH v2] rpc: Remove keepalive_required option

On Mon, Aug 03, 2015 at 09:27:37AM +0100, Daniel P. Berrange wrote: >On Mon, Aug 03, 2015 at 09:05:53AM +0200, Martin Kletzander wrote: >>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(a)redhat.com&gt; >>--- >>v2: >> - Added a new daemontest file with current input/output. >> >> daemon/libvirtd-config.c | 4 ---- >> daemon/libvirtd-config.h | 2 -- >> daemon/libvirtd.aug | 2 -- >> daemon/libvirtd.c | 2 -- >> daemon/libvirtd.conf | 7 ------ >> 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 --- >> ....json => input-data-no-keepalive-required.json} | 2 -- >> .../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 - >> ...json => output-data-no-keepalive-required.json} | 2 -- >> tests/virnetdaemontest.c | 2 +- >> 20 files changed, 5 insertions(+), 67 deletions(-) >> copy tests/virnetdaemondata/{input-data-admin-nomdns.json => input-data-no-keepalive-required.json} (96%) >> copy tests/virnetdaemondata/{input-data-admin-nomdns.json => output-data-no-keepalive-required.json} (96%) >> > >>diff --git a/daemon/libvirtd.aug b/daemon/libvirtd.aug >>index a70aa1dddf90..7c7992dd0568 100644 >>--- a/daemon/libvirtd.aug >>+++ b/daemon/libvirtd.aug >>@@ -79,11 +79,9 @@ module Libvirtd = >> >> let keepalive_entry = int_entry "keepalive_interval" >> | int_entry "keepalive_count" >>- | bool_entry "keepalive_required" >> >> let admin_keepalive_entry = int_entry "admin_keepalive_interval" >> | int_entry "admin_keepalive_count" >>- | bool_entry "admin_keepalive_required" >> >> let misc_entry = str_entry "host_uuid" >> > >We should not remove entries from this file in general as it will >cause augeas to fail to load any existing files with the config >option present. > So you are suggesting leaving them just here, but removing them from everywhere else, right? I'll send a v3 if that's the case.