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

[Christophe de Dinechin]

> 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.

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
> 
> Frediano
> _______________________________________________
> Spice-devel mailing list
> Spice-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel