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

[Daniel P. Berrange]

On Mon, Feb 27, 2017 at 12:44:27PM +0100, Christophe Fergeau wrote:
> On Mon, Feb 27, 2017 at 06:39:14AM -0500, Frediano Ziglio wrote:
> > > 
> > > On Wed, Feb 15, 2017 at 11:24:37AM -0500, Frediano Ziglio wrote:
> > > > > 
> > > > > 
> > > > > > 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)
> > > 
> > > In my opinion, a library really should not be displaying anything on
> > > stdout by default, and as little as possible on stderr. "We print this
> > > debugging code to stderr because we want it to show up in the log by
> > > default" does not sound very compelling. Why this debugging code and
> > > not eg qxl display debugging code? Next step is suggesting to always
> > > output all debugging code to stderr, but then we'd get too much :)
> > > 
> > > I'd just silence all the debugging code, or remove it, and work on a way
> > > of making it easy to enable/filter/... if what we have now is not good
> > > enough.
> > > 
> > > Christophe
> > > 
> > 
> > Sure, however your comment is going OT.
> > My comments was specific to migration problems.
> > 
> 
> And I don't think debug for migration problems should be anything
> special. It has no good reason to go to stderr.

Agreed, libraries should never unconditionally dump debug output to either
stdout or stderr. This is particularly true if any of the debug logs are
possible to trigger by the guest OS, as it could be used to flood the host
logs. So I agree with Christophe that there should be an explicit opt-in
to turn on debugging output, either via an environment variable, or via
an API call QEMU can make.

Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://entangle-photo.org       -o-    http://search.cpan.org/~danberr/ :|