On Wed, Jul 04, 2018 at 12:29:15PM -0400, Frediano Ziglio wrote: > > > > On Wed, Jul 04, 2018 at 11:38:14AM -0400, Frediano Ziglio wrote: > > > Another question is however "Are we going to use g_critical as > > > g_critical?". It sounds a tricky question. Let say that a new person > > > starts to look at the code and knows GLib. He see g_critical and > > > think "well, this by default log a critical warning and continue" > > > but instead on Spice is always fatal. > > > > Unless I am confused, g_critical() should have the usual default > > behaviour, and spice_critical() aborts, see > > test_spice_abort_level_g_warning and the following tests > > https://gitlab.freedesktop.org/spice/spice-common/blob/master/tests/test-logging.c#L62 > > > > Christophe > > > > But you suggested c3d to use g_critical instead of spice_critical, > isn't it confusing? In the context of c3d's patch, the added spice_critical calls should not be fatal, so g_critical is fine. > > Forgot about telling what I think about logging and g_XXX vs spice_XXX. > I agree we should use a single API to avoid confusion but should be > consistent, not introducing free regressions so spice_critical -> g_error > and spice_return_if_fail/spice_return_val_if_fail/spice_assert -> g_assert > (making sure it's never disabled!). If we replace preexisting logging calls and err on the very safe side, yes I agree. But I'd prefer that we don't switch everything with sed, and audit each call instead, I suspect most of the time not asserting will be ok. Christophe
Attachment:
signature.asc
Description: PGP signature
_______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/spice-devel