On Fri, Nov 27, 2015 at 10:59:57AM -0600, Jonathon Jongsma wrote:
> > +
> > +static void spice_log_set_abort_level(void)
> > +{
> > if (abort_level == -1) {
> > - abort_level = getenv("SPICE_ABORT_LEVEL") ?
> > atoi(getenv("SPICE_ABORT_LEVEL")) : SPICE_ABORT_LEVEL_DEFAULT;
> > + char *abort_str = getenv("SPICE_ABORT_LEVEL");
> > + if (abort_str != NULL) {
> > + GLogLevelFlags glib_abort_level;
> > + g_warning("Setting SPICE_ABORT_LEVEL is deprecated, use G_DEBUG
> > instead");
> > + abort_level = atoi(getenv("SPICE_ABORT_LEVEL"));
> > + glib_abort_level = spice_log_level_to_glib(abort_level);
> > + if (glib_abort_level != 0) {
> > + g_log_set_always_fatal(glib_abort_level);
>
>
> I don't think this will do what you are expecting. I think you need to pass a
> mask of all levels that you want to be fatal. So if you pass G_LOG_LEVEL_WARNING
> to this function, G_LOG_LEVEL_CRITICAL will not be fatal, which is unexpected.
Ah indeed, I did not test this situation, thanks!
>
>
> > + }
> > + } else {
> > + abort_level = SPICE_ABORT_LEVEL_DEFAULT;
> > + }
> > }
> > +}
> >
> > - if (debug_level < (int) log_level)
> > - return;
> > +static void spice_logv(const char *log_domain,
> > + SpiceLogLevel log_level,
> > + const char *strloc,
> > + const char *function,
> > + const char *format,
> > + va_list args)
> > +{
> > + GString *log_msg;
> > + static gsize logging_initialized = FALSE;
> >
> > - fprintf(stderr, "(%s:%d): ", getenv("_"), getpid());
> > -
> > - if (log_domain) {
> > - fprintf(stderr, "%s-", log_domain);
> > - }
> > - if (level) {
> > - fprintf(stderr, "%s **: ", level);
> > + g_return_if_fail(spice_log_level_to_glib(log_level) != 0);
> > + if (g_once_init_enter(&logging_initialized)) {
> > + spice_log_set_debug_level();
> > + spice_log_set_abort_level();
> > + g_once_init_leave (&logging_initialized, TRUE);
> > }
> > +
> > + log_msg = g_string_new(NULL);
> > if (strloc && function) {
> > - fprintf(stderr, "%s:%s: ", strloc, function);
> > + g_string_append_printf(log_msg, "%s:%s: ", strloc, function);
> > }
> > if (format) {
> > - vfprintf(stderr, format, args);
> > + g_string_append_vprintf(log_msg, format, args);
> > }
> > -
> > - fprintf(stderr, "\n");
> > + g_log(log_domain, spice_log_level_to_glib(log_level), "%s", log_msg
> > ->str);
> > + g_string_free(log_msg, TRUE);
>
>
> It's a bit unfortunate that we need to do an extra allocation here. Not sure
> whether it will make a real difference or not.
Having the extra allocation is not strictly required if we don't mind
not having the filename/function/line number. I kept them because in the
end, I realized we cannot simply do
#define spice_warning g_warning
but we have to have a more complicated macro. Having a more complicated
macro allows us to keep the line numbers, so I added this GString code.
I don't mind getting rid of it.
The reason for not being able to do
#define spice_warning g_warning
is because we need to set up the fallback between SPICE_*_LEVEL and
glib, and because we need to have different abort behaviours between
spice_critical/g_critical/g_return_if_fail/spice_return_if_fail.
Christophe
Attachment:
signature.asc
Description: PGP signature
_______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/spice-devel