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

[Daniel P. Berrange]

On Mon, Aug 03, 2015 at 11:43:35AM +0200, Martin Kletzander wrote:
> 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 at redhat.com>
> >>---
> >>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.

Yes, that's what we did with the 'log_buffer' setting we removed in
the past. Actually we left it in libvirtd.conf too with a comment
saying it was no longer used, for sake of historical reference

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 :|