Re: [Qemu-devel] [PATCH] spice: Make logging printing go through QEMU
Patchew URL:
https://patchew.org/QEMU/20181211143745.17342-1-cferg...@redhat.com/
Hi,
This series seems to have some coding style problems. See output below for
more information:
Message-id: 20181211143745.17342-1-cferg...@redhat.com
Subject: [Qemu-devel] [PATCH] spice: Make logging printing go through QEMU
Type: series
=== TEST SCRIPT BEGIN ===
#!/bin/bash
BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
commits="$(git log --format=%H --reverse $BASE..)"
for c in $commits; do
echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..."
if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
failed=1
echo
fi
n=$((n+1))
done
exit $failed
=== TEST SCRIPT END ===
Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
Switched to a new branch 'test'
eba6c0d spice: Make logging printing go through QEMU
=== OUTPUT BEGIN ===
Checking PATCH 1/1: spice: Make logging printing go through QEMU...
ERROR: switch and case should be at the same indent
#33: FILE: ui/spice-core.c:640:
+switch (log_level & G_LOG_LEVEL_MASK) {
+case G_LOG_LEVEL_DEBUG:
[...]
+case G_LOG_LEVEL_INFO:
[...]
+case G_LOG_LEVEL_MESSAGE:
[...]
+case G_LOG_LEVEL_WARNING:
[...]
+case G_LOG_LEVEL_CRITICAL:
[...]
+case G_LOG_LEVEL_ERROR:
total: 1 errors, 0 warnings, 37 lines checked
Your patch has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
=== OUTPUT END ===
Test command exited with code: 1
The full log is available at
http://patchew.org/logs/20181211143745.17342-1-cferg...@redhat.com/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-de...@redhat.com
Re: [Qemu-devel] [PATCH] spice: Make logging printing go through QEMU
On Tue, Dec 11, 2018 at 03:37:45PM +0100, Christophe Fergeau wrote:
> Since spice 0.14.1, it's possible to use g_log_set_default_handler() to
> use a custom function to print spice-server's logs, which gives more
> consistent log output.
>
> With older spice versions, this is not going to work as expected, but
> will not have any ill effect, so this call is not conditional on spice
> version.
>
> Since this added g_log_set_default_handler() will bridge glib logging
> and QEMU logging, the call might fit better in a more generic place.
Yeah, I think this is the kind of thing that could go early in
the main() method.
It might also be relevant to integrate it into unit tests and
other tools (qemu-img/nbd/io/etc).
>
> Signed-off-by: Christophe Fergeau
> ---
> ui/spice-core.c | 25 +
> 1 file changed, 25 insertions(+)
>
> diff --git a/ui/spice-core.c b/ui/spice-core.c
> index ebaae24643..443a2b3d32 100644
> --- a/ui/spice-core.c
> +++ b/ui/spice-core.c
> @@ -632,6 +632,30 @@ static void vm_change_state_handler(void *opaque, int
> running,
> }
> }
>
> +static void qemu_log_func(const gchar *log_domain,
> + GLogLevelFlags log_level,
> + const gchar *message,
> + gpointer user_data)
> +{
> +switch (log_level & G_LOG_LEVEL_MASK) {
> +case G_LOG_LEVEL_DEBUG:
> +break;
> +case G_LOG_LEVEL_INFO:
> +/* Fall through */
> +case G_LOG_LEVEL_MESSAGE:
> +info_report("%s", message);
> +break;
> +case G_LOG_LEVEL_WARNING:
> +/* Fall through */
> +case G_LOG_LEVEL_CRITICAL:
> +warn_report("%s", message);
IIUC, CRITICAL & ERROR are both reporting errors, the
only difference is that LEVEL_ERROR results in the
process being terminated immediately.
Anyway, I think this means G_LOG_LEVEL_CRITICAL
should use erorr_report too. Only G_LOG_LEVEL_WARNING
should use warn_report
> +break;
> +case G_LOG_LEVEL_ERROR:
> +error_report("%s", message);
> +break;
> +}
> +}
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 :|
[Qemu-devel] [PATCH] spice: Make logging printing go through QEMU
Since spice 0.14.1, it's possible to use g_log_set_default_handler() to
use a custom function to print spice-server's logs, which gives more
consistent log output.
With older spice versions, this is not going to work as expected, but
will not have any ill effect, so this call is not conditional on spice
version.
Since this added g_log_set_default_handler() will bridge glib logging
and QEMU logging, the call might fit better in a more generic place.
Signed-off-by: Christophe Fergeau
---
ui/spice-core.c | 25 +
1 file changed, 25 insertions(+)
diff --git a/ui/spice-core.c b/ui/spice-core.c
index ebaae24643..443a2b3d32 100644
--- a/ui/spice-core.c
+++ b/ui/spice-core.c
@@ -632,6 +632,30 @@ static void vm_change_state_handler(void *opaque, int
running,
}
}
+static void qemu_log_func(const gchar *log_domain,
+ GLogLevelFlags log_level,
+ const gchar *message,
+ gpointer user_data)
+{
+switch (log_level & G_LOG_LEVEL_MASK) {
+case G_LOG_LEVEL_DEBUG:
+break;
+case G_LOG_LEVEL_INFO:
+/* Fall through */
+case G_LOG_LEVEL_MESSAGE:
+info_report("%s", message);
+break;
+case G_LOG_LEVEL_WARNING:
+/* Fall through */
+case G_LOG_LEVEL_CRITICAL:
+warn_report("%s", message);
+break;
+case G_LOG_LEVEL_ERROR:
+error_report("%s", message);
+break;
+}
+}
+
void qemu_spice_init(void)
{
QemuOpts *opts = QTAILQ_FIRST(&qemu_spice_opts.head);
@@ -647,6 +671,7 @@ void qemu_spice_init(void)
spice_wan_compression_t wan_compr;
bool seamless_migration;
+g_log_set_default_handler(qemu_log_func, NULL);
qemu_thread_get_self(&me);
if (!opts) {
--
2.19.2
