[PATCH 0/3] Virtlogd fixes

Please see individual patches for justification. Peter Krempa (3): rpc: client: Don't check return value of virNetMessageNew rpc: Don't warn about "max_client_requests" in single-threaded daemons virLogCleanerShutdown: Don't call g_regex_unref on NULL regex src/logging/log_cleaner.c | 3 +-- src/remote/libvirtd.conf.in | 1 + src/rpc/virnetserverclient.c | 14 ++++++-------- 3 files changed, 8 insertions(+), 10 deletions(-) -- 2.39.1

virNetServerClientDispatchRead checked the return value but it's not necessary any more as it can't return NULL nowadays. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/rpc/virnetserverclient.c | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/src/rpc/virnetserverclient.c b/src/rpc/virnetserverclient.c index c9a4eb521e..b5c764b1b0 100644 --- a/src/rpc/virnetserverclient.c +++ b/src/rpc/virnetserverclient.c @@ -1257,13 +1257,10 @@ static virNetMessage *virNetServerClientDispatchRead(virNetServerClient *client) /* Possibly need to create another receive buffer */ if (client->nrequests < client->nrequests_max) { - if (!(client->rx = virNetMessageNew(true))) { - client->wantClose = true; - } else { - client->rx->bufferLength = VIR_NET_MESSAGE_LEN_MAX; - client->rx->buffer = g_new0(char, client->rx->bufferLength); - client->nrequests++; - } + client->rx = virNetMessageNew(true); + client->rx->bufferLength = VIR_NET_MESSAGE_LEN_MAX; + client->rx->buffer = g_new0(char, client->rx->bufferLength); + client->nrequests++; } else if (!client->nrequests_warning) { client->nrequests_warning = true; VIR_WARN("Client hit max requests limit %zd. This may result " -- 2.39.1

On Wed, Feb 15, 2023 at 11:06:09AM +0100, Peter Krempa wrote:
virNetServerClientDispatchRead checked the return value but it's not necessary any more as it can't return NULL nowadays.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/rpc/virnetserverclient.c | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-)
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> With regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|

The warning about max_client_requests is hit inside virtlogd every time a VM starts which spams the logs. Emit the warning only when the client request limit is not 1 and add a warning into the daemon config to not configure it too low instead. Fixes: 031878c2364 Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=2145188 Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/remote/libvirtd.conf.in | 1 + src/rpc/virnetserverclient.c | 3 ++- 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/src/remote/libvirtd.conf.in b/src/remote/libvirtd.conf.in index 80a98b1529..32a680317a 100644 --- a/src/remote/libvirtd.conf.in +++ b/src/remote/libvirtd.conf.in @@ -374,6 +374,7 @@ # connection. To avoid one client monopolizing the server # this should be a small fraction of the global max_workers # parameter. +# Setting this too low may cause keepalive timeouts. #max_client_requests = 5 # Same processing controls, but this time for the admin interface. diff --git a/src/rpc/virnetserverclient.c b/src/rpc/virnetserverclient.c index b5c764b1b0..bdb3552c5d 100644 --- a/src/rpc/virnetserverclient.c +++ b/src/rpc/virnetserverclient.c @@ -1261,7 +1261,8 @@ static virNetMessage *virNetServerClientDispatchRead(virNetServerClient *client) client->rx->bufferLength = VIR_NET_MESSAGE_LEN_MAX; client->rx->buffer = g_new0(char, client->rx->bufferLength); client->nrequests++; - } else if (!client->nrequests_warning) { + } else if (!client->nrequests_warning && + client->nrequests_max > 1) { client->nrequests_warning = true; VIR_WARN("Client hit max requests limit %zd. This may result " "in keep-alive timeouts. Consider tuning the " -- 2.39.1

On Wed, Feb 15, 2023 at 11:06:10AM +0100, Peter Krempa wrote:
The warning about max_client_requests is hit inside virtlogd every time a VM starts which spams the logs.
Emit the warning only when the client request limit is not 1 and add a warning into the daemon config to not configure it too low instead.
Fixes: 031878c2364 Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=2145188 Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/remote/libvirtd.conf.in | 1 + src/rpc/virnetserverclient.c | 3 ++- 2 files changed, 3 insertions(+), 1 deletion(-)
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> With regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|

Shutdown of virtlogd prints: (process:54742): GLib-CRITICAL **: 11:00:40.873: g_regex_unref: assertion 'regex != NULL' failed Use g_clear_pointer instead which prevents it in the NULL case. Fixes: 69eeef5dfbf Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/logging/log_cleaner.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/logging/log_cleaner.c b/src/logging/log_cleaner.c index bb8f719f1b..38f818f177 100644 --- a/src/logging/log_cleaner.c +++ b/src/logging/log_cleaner.c @@ -264,6 +264,5 @@ virLogCleanerShutdown(virLogHandler *handler) handler->cleanup_log_timer = -1; } - g_regex_unref(log_regex); - log_regex = NULL; + g_clear_pointer(&log_regex, g_regex_unref); } -- 2.39.1

On Wed, Feb 15, 2023 at 11:06:11AM +0100, Peter Krempa wrote:
Shutdown of virtlogd prints:
(process:54742): GLib-CRITICAL **: 11:00:40.873: g_regex_unref: assertion 'regex != NULL' failed
Use g_clear_pointer instead which prevents it in the NULL case.
Fixes: 69eeef5dfbf Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/logging/log_cleaner.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> With regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
participants (2)
-
Daniel P. Berrangé
-
Peter Krempa