[spice-gtk] Replace spice_printerr() with g_warning()

Submitted by Christophe Fergeau on June 29, 2018, 3:41 p.m.

Details

Message ID 20180629154142.GF31369@natto.ory.fergeau.eu
State New
Headers show
Series "Replace spice_printerr() with g_warning()" ( rev: 2 ) in Spice

Not browsing as part of any series.

Commit Message

Christophe Fergeau June 29, 2018, 3:41 p.m.
On Thu, Jun 28, 2018 at 04:38:21PM +0100, Frediano Ziglio wrote:
> The remaining occurrences of spice_printerr() are warnings when
> something unexpected happens, they can be replaced with g_warning() so
> that users of spice-server can redirect them with
> g_log_set_default_handler().

Maybe squash this in?

Either way, Acked-by: Christophe Fergeau <cfergeau@redhat.com>



> 
> Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
> ---
>  src/channel-display.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> I know... I'm so lazy I didn't even came out with my
> own commit message...
> 
> diff --git a/src/channel-display.c b/src/channel-display.c
> index 8fb4dee9..44ba0439 100644
> --- a/src/channel-display.c
> +++ b/src/channel-display.c
> @@ -1211,7 +1211,7 @@ static void report_invalid_stream(SpiceChannel *channel, uint32_t id)
>          /* Send a special stream report (UINT_MAX dropped frames out of zero)
>           * to indicate there is no such stream.
>           */
> -        spice_printerr("notify the server that stream %u does not exist", id);
> +        g_warning("notify the server that stream %u does not exist", id);
>          memset(&report, 0, sizeof(report));
>          report.stream_id = id;
>          report.num_frames = 0;
> @@ -1269,7 +1269,7 @@ static display_stream *display_stream_create(SpiceChannel *channel,
>          break;
>      }
>      if (st->video_decoder == NULL) {
> -        spice_printerr("could not create a video decoder for codec %u", codec_type);
> +        g_warning("could not create a video decoder for codec %u", codec_type);
>          g_clear_pointer(&st, display_stream_destroy);
>      }
>      return st;
> @@ -1310,7 +1310,7 @@ static void display_handle_stream_create(SpiceChannel *channel, SpiceMsgIn *in)
>                                                 op->flags, op->codec_type,
>                                                 &op->dest, &op->clip);
>      if (c->streams[op->id] == NULL) {
> -        spice_printerr("could not create the %u video stream", op->id);
> +        g_warning("could not create the %u video stream", op->id);
>          destroy_stream(channel, op->id);
>          report_invalid_stream(channel, op->id);
>      }
> -- 
> 2.17.1
> 
> _______________________________________________
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel

Patch hide | download patch | download mbox

diff --git a/src/channel-display.c b/src/channel-display.c
index 44ba0439..1c28aed1 100644
--- a/src/channel-display.c
+++ b/src/channel-display.c
@@ -1211,7 +1211,7 @@  static void report_invalid_stream(SpiceChannel *channel, uint32_t id)
         /* Send a special stream report (UINT_MAX dropped frames out of zero)
          * to indicate there is no such stream.
          */
-        g_warning("notify the server that stream %u does not exist", id);
+        g_warning("%s: notify the server that stream %u does not exist", channel->priv->name, id);
         memset(&report, 0, sizeof(report));
         report.stream_id = id;
         report.num_frames = 0;
@@ -1269,7 +1269,8 @@  static display_stream *display_stream_create(SpiceChannel *channel,
         break;
     }
     if (st->video_decoder == NULL) {
-        g_warning("could not create a video decoder for codec %u", codec_type);
+        g_warning("%s: could not create a video decoder for codec %u",
+                  channel->priv->name, codec_type);
         g_clear_pointer(&st, display_stream_destroy);
     }
     return st;
@@ -1310,7 +1311,7 @@  static void display_handle_stream_create(SpiceChannel *channel, SpiceMsgIn *in)
                                                op->flags, op->codec_type,
                                                &op->dest, &op->clip);
     if (c->streams[op->id] == NULL) {
-        g_warning("could not create the %u video stream", op->id);
+        g_warning("%s: could not create the %u video stream", channel->priv->name, op->id);
         destroy_stream(channel, op->id);
         report_invalid_stream(channel, op->id);
     }

Comments

> 
> On Thu, Jun 28, 2018 at 04:38:21PM +0100, Frediano Ziglio wrote:
> > The remaining occurrences of spice_printerr() are warnings when
> > something unexpected happens, they can be replaced with g_warning() so
> > that users of spice-server can redirect them with
> > g_log_set_default_handler().
> 
> Maybe squash this in?
> 
> Either way, Acked-by: Christophe Fergeau <cfergeau@redhat.com>
> 

I can do a follow up but IMO they don't see to help much, surely
is the display channel and knowing the id of which one means
that you have multiple video running on the same VM.
I mean, looks like a peculiar situation you don't want to
have so many detail in the normal case...

> diff --git a/src/channel-display.c b/src/channel-display.c
> index 44ba0439..1c28aed1 100644
> --- a/src/channel-display.c
> +++ b/src/channel-display.c
> @@ -1211,7 +1211,7 @@ static void report_invalid_stream(SpiceChannel
> *channel, uint32_t id)
>          /* Send a special stream report (UINT_MAX dropped frames out of
>          zero)
>           * to indicate there is no such stream.
>           */
> -        g_warning("notify the server that stream %u does not exist", id);
> +        g_warning("%s: notify the server that stream %u does not exist",
> channel->priv->name, id);
>          memset(&report, 0, sizeof(report));
>          report.stream_id = id;
>          report.num_frames = 0;
> @@ -1269,7 +1269,8 @@ static display_stream
> *display_stream_create(SpiceChannel *channel,
>          break;
>      }
>      if (st->video_decoder == NULL) {
> -        g_warning("could not create a video decoder for codec %u",
> codec_type);
> +        g_warning("%s: could not create a video decoder for codec %u",
> +                  channel->priv->name, codec_type);
>          g_clear_pointer(&st, display_stream_destroy);
>      }
>      return st;
> @@ -1310,7 +1311,7 @@ static void display_handle_stream_create(SpiceChannel
> *channel, SpiceMsgIn *in)
>                                                 op->flags, op->codec_type,
>                                                 &op->dest, &op->clip);
>      if (c->streams[op->id] == NULL) {
> -        g_warning("could not create the %u video stream", op->id);
> +        g_warning("%s: could not create the %u video stream",
> channel->priv->name, op->id);
>          destroy_stream(channel, op->id);
>          report_invalid_stream(channel, op->id);
>      }
> 
> 
...
On Fri, Jun 29, 2018 at 02:56:06PM -0400, Frediano Ziglio wrote:
> > 
> > On Thu, Jun 28, 2018 at 04:38:21PM +0100, Frediano Ziglio wrote:
> > > The remaining occurrences of spice_printerr() are warnings when
> > > something unexpected happens, they can be replaced with g_warning() so
> > > that users of spice-server can redirect them with
> > > g_log_set_default_handler().
> > 
> > Maybe squash this in?
> > 
> > Either way, Acked-by: Christophe Fergeau <cfergeau@redhat.com>
> > 
> 
> I can do a follow up but IMO they don't see to help much, surely
> is the display channel and knowing the id of which one means
> that you have multiple video running on the same VM.
> I mean, looks like a peculiar situation you don't want to
> have so many detail in the normal case...

Imo, we should always prefix log messages with the channel name when
they are related to a channel. This way when I look at eg a sound
channel issue, if I get this warning at the same time, I don't have to
wonder if 'stream' relates to sound somehow, or if it's something
different.

Christophe
> 
> On Fri, Jun 29, 2018 at 02:56:06PM -0400, Frediano Ziglio wrote:
> > > 
> > > On Thu, Jun 28, 2018 at 04:38:21PM +0100, Frediano Ziglio wrote:
> > > > The remaining occurrences of spice_printerr() are warnings when
> > > > something unexpected happens, they can be replaced with g_warning() so
> > > > that users of spice-server can redirect them with
> > > > g_log_set_default_handler().
> > > 
> > > Maybe squash this in?
> > > 
> > > Either way, Acked-by: Christophe Fergeau <cfergeau@redhat.com>
> > > 
> > 
> > I can do a follow up but IMO they don't see to help much, surely
> > is the display channel and knowing the id of which one means
> > that you have multiple video running on the same VM.
> > I mean, looks like a peculiar situation you don't want to
> > have so many detail in the normal case...
> 
> Imo, we should always prefix log messages with the channel name when
> they are related to a channel. This way when I look at eg a sound
> channel issue, if I get this warning at the same time, I don't have to
> wonder if 'stream' relates to sound somehow, or if it's something
> different.
> 
> Christophe
> 

Oh, so was a coherence suggestion.
Not much to do with a API change, isn't it?
I don't think you as a person will have problems, but you as a grep/whatever
user could benefit by coherences like these

Frediano