[Spice-devel] [PATCH] log: add not fatal spice_return function

[Victor Toso]

On Fri, Nov 20, 2015 at 08:53:19AM -0600, Jonathon Jongsma wrote:
> On Fri, 2015-11-20 at 12:31 +0000, Frediano Ziglio wrote:
> > Due to implementation details spice_return is by default fatal (program
> > is aborting). This is quite confusing but changing the current macros
> > would possibly break existing code.
> > Add not fatal (at least by default) macros. They use warning level
> > instead of critical.
> > This is also compatible with Glib macros.
> > 
> > Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
> > ---
> >  common/log.h | 18 ++++++++++++++++++
> >  1 file changed, 18 insertions(+)
> > 
> > diff --git a/common/log.h b/common/log.h
> > index d9e6023..fdfdfa4 100644
> > --- a/common/log.h
> > +++ b/common/log.h
> > @@ -73,6 +73,24 @@ void spice_log(const char *log_domain,
> >  } SPICE_STMT_END
> >  #endif
> >  
> > +#ifndef spice_return_if_fail_warning
> > +#define spice_return_if_fail_warning(x) SPICE_STMT_START {              \
> > +    if SPICE_LIKELY(x) { } else {                                       \
> > +        spice_warning("condition `%s' failed", #x);                     \
> > +        return;                                                         \
> > +    }                                                                   \
> > +} SPICE_STMT_END
> > +#endif
> > +
> > +#ifndef spice_return_val_if_fail_warning
> > +#define spice_return_val_if_fail_warning(x, val) SPICE_STMT_START {     \
> > +    if SPICE_LIKELY(x) { } else {                                       \
> > +        spice_warning("condition `%s' failed", #x);                     \
> > +        return (val);                                                   \
> > +    }                                                                   \
> > +} SPICE_STMT_END
> > +#endif
> > +
> >  #ifndef spice_warn_if_reached
> >  #define spice_warn_if_reached() SPICE_STMT_START {                      \
> >      spice_log(SPICE_LOG_DOMAIN, SPICE_LOG_LEVEL_WARNING, SPICE_STRLOC,
> > __FUNCTION__, "should not be reached"); \
>
>
> I don't know. I think I agree with Christophe and Marc-Andre that adding more
> spice-specific logging functions just confuses the issue. I'd rather just use
> g_return_if_fail for new code and gradually go through all other uses of
> spice_return_if and either convert them to g_return_if or to spice_assert.

This is confusing to me due the fact that we want to use g_return_if*
here but we are not changing the other glib functions for patches that
are being reviewed, tested and merged.

I thought that this was already decided in previous reviews that we are
not changing to glib *now* but we will do it later and because of that, I
do agree with Frediano's patch.

Now, as stated here [0], I would *really* prefer using glib functions
for all new code and when merge is done, doing the sed.

[0]
http://lists.freedesktop.org/archives/spice-devel/2015-November/023364.html

Let's please decided this as soon as possible.

If g_return_if_fail is fine, why not *avoiding* the usage of spice_*
functions that are in glib? (Log / checking / memory / ..)

Best,
  - toso