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

Re: [libvirt] [PATCH 2/2] daemon: Introduce max_anonymous_clients



On Mon, Dec 09, 2013 at 03:35:53PM +0100, Michal Privoznik wrote:
> https://bugzilla.redhat.com/show_bug.cgi?id=981729
>
> This config tunable allows users to determine the maximum number of
> accepted but yet not authenticated users.
>
> Signed-off-by: Michal Privoznik <mprivozn redhat com>
> ---
>  daemon/libvirtd-config.c    |  1 +
>  daemon/libvirtd-config.h    |  1 +
>  daemon/libvirtd.aug         |  1 +
>  daemon/libvirtd.c           |  1 +
>  daemon/libvirtd.conf        |  3 +++
>  daemon/test_libvirtd.aug.in |  1 +
>  src/locking/lock_daemon.c   |  4 ++--
>  src/lxc/lxc_controller.c    |  2 +-
>  src/rpc/virnetserver.c      | 37 +++++++++++++++++++++++++++++++++++--
>  src/rpc/virnetserver.h      |  1 +
>  10 files changed, 47 insertions(+), 5 deletions(-)
>
> diff --git a/daemon/libvirtd-config.c b/daemon/libvirtd-config.c
> index c816fda..04482c5 100644
> --- a/daemon/libvirtd-config.c
> +++ b/daemon/libvirtd-config.c
> @@ -415,6 +415,7 @@ daemonConfigLoadOptions(struct daemonConfig *data,
>      GET_CONF_INT(conf, filename, max_workers);
>      GET_CONF_INT(conf, filename, max_clients);
>      GET_CONF_INT(conf, filename, max_queued_clients);
> +    GET_CONF_INT(conf, filename, max_anonymous_clients);
>
>      GET_CONF_INT(conf, filename, prio_workers);
>
> diff --git a/daemon/libvirtd-config.h b/daemon/libvirtd-config.h
> index a24d5d2..66dc80b 100644
> --- a/daemon/libvirtd-config.h
> +++ b/daemon/libvirtd-config.h
> @@ -64,6 +64,7 @@ struct daemonConfig {
>      int max_workers;
>      int max_clients;
>      int max_queued_clients;
> +    int max_anonymous_clients;
>
>      int prio_workers;
>
> diff --git a/daemon/libvirtd.aug b/daemon/libvirtd.aug
> index 70fce5c..5a0807c 100644
> --- a/daemon/libvirtd.aug
> +++ b/daemon/libvirtd.aug
> @@ -57,6 +57,7 @@ module Libvirtd =
>                          | int_entry "max_workers"
>                          | int_entry "max_clients"
>                          | int_entry "max_queued_clients"
> +                        | int_entry "max_anonymous_clients"
>                          | int_entry "max_requests"
>                          | int_entry "max_client_requests"
>                          | int_entry "prio_workers"
> diff --git a/daemon/libvirtd.c b/daemon/libvirtd.c
> index 49c42ad..61c706c 100644
> --- a/daemon/libvirtd.c
> +++ b/daemon/libvirtd.c
> @@ -1367,6 +1367,7 @@ int main(int argc, char **argv) {
>                                  config->max_workers,
>                                  config->prio_workers,
>                                  config->max_clients,
> +                                config->max_anonymous_clients,
>                                  config->keepalive_interval,
>                                  config->keepalive_count,
>                                  !!config->keepalive_required,
> diff --git a/daemon/libvirtd.conf b/daemon/libvirtd.conf
> index 5353927..28d3735 100644
> --- a/daemon/libvirtd.conf
> +++ b/daemon/libvirtd.conf
> @@ -263,6 +263,9 @@
>  # connection succeeds.
>  #max_queued_clients = 1000
>
> +# The maximum length of queue of accepted but not yet not
> +# authenticated clients.
> +#max_anonymous_clients = 20
>

Please mention what's the default here.

>  # The minimum limit sets the number of workers to start up
>  # initially. If the number of active clients exceeds this,
> diff --git a/daemon/test_libvirtd.aug.in b/daemon/test_libvirtd.aug.in
> index a7e8515..b03451c 100644
> --- a/daemon/test_libvirtd.aug.in
> +++ b/daemon/test_libvirtd.aug.in
> @@ -36,6 +36,7 @@ module Test_libvirtd =
>          }
>          { "max_clients" = "20" }
>          { "max_queued_clients" = "1000" }
> +        { "max_anonymous_clients" = "20" }
>          { "min_workers" = "5" }
>          { "max_workers" = "20" }
>          { "prio_workers" = "5" }
> diff --git a/src/locking/lock_daemon.c b/src/locking/lock_daemon.c
> index 35ccb4e..c97bc61 100644
> --- a/src/locking/lock_daemon.c
> +++ b/src/locking/lock_daemon.c
> @@ -143,8 +143,8 @@ virLockDaemonNew(virLockDaemonConfigPtr config, bool privileged)
>      }
>
>      if (!(lockd->srv = virNetServerNew(1, 1, 0, config->max_clients,
> -                                       -1, 0,
> -                                       false, NULL,
> +                                       config->max_clients,
> +                                       -1, 0, false, NULL,

Since the default (when there's nothing in the config) is 0, it should
mean the feature is disabled).  If that's what you've intended,
wouldn't it make more sense to use 0 so it's visible that the feature
is not in effect?

>                                         virLockDaemonClientNew,
>                                         virLockDaemonClientPreExecRestart,
>                                         virLockDaemonClientFree,
> diff --git a/src/lxc/lxc_controller.c b/src/lxc/lxc_controller.c
> index c013147..578a135 100644
> --- a/src/lxc/lxc_controller.c
> +++ b/src/lxc/lxc_controller.c
> @@ -736,7 +736,7 @@ static int virLXCControllerSetupServer(virLXCControllerPtr ctrl)
>                      LXC_STATE_DIR, ctrl->name) < 0)
>          return -1;
>
> -    if (!(ctrl->server = virNetServerNew(0, 0, 0, 1,
> +    if (!(ctrl->server = virNetServerNew(0, 0, 0, 1, 1,

Same here?

>                                           -1, 0, false,
>                                           NULL,
>                                           virLXCControllerClientPrivateNew,
> diff --git a/src/rpc/virnetserver.c b/src/rpc/virnetserver.c
> index 1b2c6d4..bbb91d4 100644
> --- a/src/rpc/virnetserver.c
> +++ b/src/rpc/virnetserver.c
> @@ -91,6 +91,7 @@ struct _virNetServer {
>      virNetServerClientPtr *clients;     /* Clients */
>      size_t nclients_max;                /* Max allowed clients count */
>      size_t nclients_unauth;             /* Unauthenticated clients count */
> +    size_t nclients_unauth_max;         /* Max allowed unauth clients count */
>
>      int keepaliveInterval;
>      unsigned int keepaliveCount;
> @@ -278,6 +279,13 @@ static int virNetServerAddClient(virNetServerPtr srv,
>      if (virNetServerClientNeedAuth(client))
>          virNetServerClientAuthLocked(srv, true);
>
> +    if (srv->nclients_unauth == srv->nclients_unauth_max) {
> +        /* Temporarily stop accepting new clients */
> +        VIR_DEBUG("Temporarily suspending services "
> +                  "due to max_anonymous_clients");
> +        virNetServerUpdateServicesLocked(srv, false);
> +    }
> +
>      if (srv->nclients == srv->nclients_max) {
>          /* Temporarily stop accepting new clients */
>          VIR_DEBUG("Temporarily suspending services due to max_clients");
> @@ -361,6 +369,7 @@ virNetServerPtr virNetServerNew(size_t min_workers,
>                                  size_t max_workers,
>                                  size_t priority_workers,
>                                  size_t max_clients,
> +                                size_t max_anonymous_clients,
>                                  int keepaliveInterval,
>                                  unsigned int keepaliveCount,
>                                  bool keepaliveRequired,
> @@ -387,6 +396,7 @@ virNetServerPtr virNetServerNew(size_t min_workers,
>          goto error;
>
>      srv->nclients_max = max_clients;
> +    srv->nclients_unauth_max = max_anonymous_clients;
>      srv->keepaliveInterval = keepaliveInterval;
>      srv->keepaliveCount = keepaliveCount;
>      srv->keepaliveRequired = keepaliveRequired;
> @@ -506,6 +516,7 @@ virNetServerPtr virNetServerNewPostExecRestart(virJSONValuePtr object,
>
>      if (!(srv = virNetServerNew(min_workers, max_clients,
>                                  priority_workers, max_clients,
> +                                max_clients,

Here it should be taken from the JSON object (and 0 if not found)
which should be populated from the save file.  Also update that save
file populating so the value is kept throughout all restarts.

>                                  keepaliveInterval, keepaliveCount,
>                                  keepaliveRequired, mdnsGroupName,
>                                  clientPrivNew, clientPrivPreExecRestart,
> @@ -1145,8 +1156,16 @@ void virNetServerRun(virNetServerPtr srv)
>                      virNetServerClientAuthLocked(srv, false);
>
>                  /* Enable services if we can accept a new client.
> -                 * The new client can be accepted if we are at the limit. */
> -                if (srv->nclients == srv->nclients_max - 1) {
> +                 * The new client can be accepted if both max_clients and
> +                 * max_anonymous_clients wouldn't get overcommitted by
> +                 * accepting it. */
> +                VIR_DEBUG("Considering re-enabling services: "
> +                          "nclients=%zu nclients_max=%zu "
> +                          "nclients_unauth=%zu nclients_unauth_max=%zu",
> +                          srv->nclients, srv->nclients_max,
> +                          srv->nclients_unauth, srv->nclients_unauth_max);
> +                if ((srv->nclients == srv->nclients_max - 1) &&
> +                    (srv->nclients_unauth < srv->nclients_unauth_max)) {

Spurious parentheses, plus even considering your squash-in, this and
similar conditions don't count on nclients_unauth_max being 0;
consider squashing this in:

diff --git a/src/rpc/virnetserver.c b/src/rpc/virnetserver.c
index 62490a7..332641e 100644
--- a/src/rpc/virnetserver.c
+++ b/src/rpc/virnetserver.c
@@ -279,7 +279,8 @@ static int virNetServerAddClient(virNetServerPtr srv,
     if (virNetServerClientNeedAuth(client))
         virNetServerClientAuthLocked(srv, true);

-    if (srv->nclients_unauth == srv->nclients_unauth_max) {
+    if (srv->nclients_unauth_max &&
+        srv->nclients_unauth == srv->nclients_unauth_max) {
         /* Temporarily stop accepting new clients */
         VIR_DEBUG("Temporarily suspending services "
                   "due to max_anonymous_clients");
@@ -1164,8 +1165,9 @@ void virNetServerRun(virNetServerPtr srv)
                           "nclients_unauth=%zu
                           nclients_unauth_max=%zu",
                           srv->nclients, srv->nclients_max,
                           srv->nclients_unauth,
                           srv->nclients_unauth_max);
-                if ((srv->nclients < srv->nclients_max) &&
-                    (srv->nclients_unauth < srv->nclients_unauth_max)) {
+                if (srv->nclients < srv->nclients_max &&
+                    (!srv->nclients_unauth_max ||
+                     srv->nclients_unauth < srv->nclients_unauth_max)) {
                     /* Now it makes sense to accept() a new
                     client. */
                     VIR_DEBUG("Re-enabling services");
                     virNetServerUpdateServicesLocked(srv, true);
@@ -1284,8 +1286,9 @@ size_t virNetServerClientAuth(virNetServerPtr srv,
               "nclients_unauth=%zu nclients_unauth_max=%zu",
               srv->nclients, srv->nclients_max,
               srv->nclients_unauth, srv->nclients_unauth_max);
-    if ((srv->nclients < srv->nclients_max) &&
-        (srv->nclients_unauth < srv->nclients_unauth_max)) {
+    if (srv->nclients < srv->nclients_max &&
+        (!srv->nclients_unauth_max ||
+         srv->nclients_unauth < srv->nclients_unauth_max)) {
         /* Now it makes sense to accept() a new client. */
         VIR_DEBUG("Re-enabling services");
         virNetServerUpdateServicesLocked(srv, true);
--

Martin

Attachment: signature.asc
Description: Digital signature


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