Re: [PATCH v3] rpc: Remove keepalive_required option

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 03.08.2015 13:19, 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.

s/untile/until/

> 
> 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@xxxxxxxxxx>
> ---
> 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
> 

ACK

Michal

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list



[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]