[Spice-devel] [spice-server v4 5/5] Change some spice_printerr() to spice_debug()

[Frediano Ziglio]

> 
> 
> > On 15 Feb 2017, at 12:45, Frediano Ziglio <fziglio at redhat.com> wrote:
> > 
> >> @@ -134,7 +134,7 @@ static void red_qxl_display_migrate(RedChannelClient
> >> *rcc)
> >>     }
> >>     g_object_get(channel, "channel-type", &type, "id", &id, NULL);
> >>     dispatcher = (Dispatcher *)g_object_get_data(G_OBJECT(channel),
> >>     "dispatcher");
> >> -    spice_printerr("channel type %u id %u", type, id);
> >> +    spice_debug("channel type %u id %u", type, id);
> >>     payload.rcc = rcc;
> >>     dispatcher_send_message(dispatcher,
> >>                             RED_WORKER_MESSAGE_DISPLAY_MIGRATE,
> > 
> > Looks like there's lot of debugging on migration.
> > Like we are not really sure the code is working.
> 
> Maybe someone once needed a lot of debug information, and kept it around in
> case things start going south. That probably means we have annotations that
> may end up being annoying for everybody debugging anything else than
> migration.
> 

I wrote that consideration thinking about customer reports.
spice_printerr goes in the logs while spice_debug not (by default).
If we have lot of bugs in migration removing from client logs could
became an issue for us. It's hard to ask to the customer to enable all
debugging (too invasive) and even if it could be enabled separately
it could be too late (as hard to reproduce for the customer) or
complicated (think about huge setups).

(CCing Jasa)

> This leads me to a meta-question: would it make sense to add “traces” to the
> spice code, i.e. dynamically configurable flags that activate sets of
> related printf. Today, we have spice “errors”, “debug” or “info”, but we
> could have finer-grained logging for specific topics, e.g. migration or
> encoding.
> 
> In Tao3D, there are topical traces like this declared here:
> https://github.com/c3d/tao-3D/blob/master/tao/traces.tbl. If you want to
> have the “font” trace activated, you simply run the program with the -tfont
> option, or set a flag from within a debugger. Within the code, traces are
> tested with something like if (TRACE(font)), e.g.
> https://github.com/c3d/tao-3D/blob/master/tao/font.cpp#L102. A special form
> a printf takes a trace name as input, e.g. we could have spice_trace(font,
> …). The cost of a not-taken trace is a mere not-taken if statement with a
> bit-field read (one trace = one bit), so the overhead is really low. This
> means that you can leave verbose debug information in place in case it helps
> addressing a specific kind of debug situation.
> 
> Would anything like this make sense for spice?
> 
> 
> >> @@ -148,7 +148,7 @@ static void red_qxl_set_cursor_peer(RedChannel
> >> *channel,
> >> RedClient *client, Reds
> >> {
> >>     RedWorkerMessageCursorConnect payload = {0,};
> >>     Dispatcher *dispatcher = (Dispatcher
> >>     *)g_object_get_data(G_OBJECT(channel), "dispatcher");
> >> -    spice_printerr("");
> >> +    spice_debug("");
> >>     payload.client = client;
> >>     payload.stream = stream;
> >>     payload.migration = migration;
> > 
> > Remove
> > 
> >> @@ -176,7 +176,7 @@ static void
> >> red_qxl_disconnect_cursor_peer(RedChannelClient *rcc)
> >>     }
> >> 
> >>     dispatcher = (Dispatcher *)g_object_get_data(G_OBJECT(channel),
> >>     "dispatcher");
> >> -    spice_printerr("");
> >> +    spice_debug("");
> >>     payload.rcc = rcc;
> >> 
> >>     dispatcher_send_message(dispatcher,
> > 
> > Remove
> > 
> >> @@ -196,7 +196,7 @@ static void red_qxl_cursor_migrate(RedChannelClient
> >> *rcc)
> >>     }
> >>     g_object_get(channel, "channel-type", &type, "id", &id, NULL);
> >>     dispatcher = (Dispatcher *)g_object_get_data(G_OBJECT(channel),
> >>     "dispatcher");
> >> -    spice_printerr("channel type %u id %u", type, id);
> >> +    spice_debug("channel type %u id %u", type, id);
> >>     payload.rcc = rcc;
> >>     dispatcher_send_message(dispatcher,
> >>                             RED_WORKER_MESSAGE_CURSOR_MIGRATE,
> >> @@ -652,7 +652,7 @@ static void red_qxl_loadvm_commands(QXLState
> >> *qxl_state,
> >> {
> >>     RedWorkerMessageLoadvmCommands payload;
> >> 
> >> -    spice_printerr("");
> >> +    spice_debug("");
> >>     payload.count = count;
> >>     payload.ext = ext;
> >>     dispatcher_send_message(qxl_state->dispatcher,
> >> diff --git a/server/smartcard.c b/server/smartcard.c
> >> index a7cc614..4d27eff 100644
> >> --- a/server/smartcard.c
> >> +++ b/server/smartcard.c
> >> @@ -213,7 +213,7 @@ static void smartcard_remove_client(RedCharDevice
> >> *self,
> >> RedClient *client)
> >>     RedCharDeviceSmartcard *dev = RED_CHAR_DEVICE_SMARTCARD(self);
> >>     RedChannelClient *rcc = RED_CHANNEL_CLIENT(dev->priv->scc);
> >> 
> >> -    spice_printerr("smartcard  dev %p, client %p", dev, client);
> >> +    spice_debug("smartcard  dev %p, client %p", dev, client);
> >>     spice_assert(dev->priv->scc &&
> >>                  red_channel_client_get_client(rcc) == client);
> >>     red_channel_client_shutdown(rcc);
> > 
> > Not sure. Not really into smartcard to have an opinion.
> > 
> >> diff --git a/server/spicevmc.c b/server/spicevmc.c
> >> index 4c9f442..2b3290a 100644
> >> --- a/server/spicevmc.c
> >> +++ b/server/spicevmc.c
> >> @@ -436,7 +436,7 @@ static void
> >> spicevmc_char_dev_remove_client(RedCharDevice
> >> *self,
> >>     RedCharDeviceSpiceVmc *vmc = RED_CHAR_DEVICE_SPICEVMC(self);
> >>     RedVmcChannel *channel = RED_VMC_CHANNEL(vmc->channel);
> >> 
> >> -    spice_printerr("vmc channel %p, client %p", channel, client);
> >> +    spice_debug("vmc channel %p, client %p", channel, client);
> >>     spice_assert(channel->rcc &&
> >>                  red_channel_client_get_client(channel->rcc) == client);
> >> 
> > 
> > Same as smartcard
> >