[libvirt] [PATCH v2] rpc: Remove keepalive_required option
Martin Kletzander
mkletzan at redhat.com
Mon Aug 3 09:43:35 UTC 2015
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.
>
>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 :|
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20150803/b0c52589/attachment-0001.sig>
More information about the libvir-list
mailing list