[Spice-devel] [PATCH v3 5/9] Add new spice-gl stubs API

[Frediano Ziglio]

> 
> Hi
> 
> On Thu, Feb 4, 2016 at 4:29 PM, Frediano Ziglio <fziglio at redhat.com> wrote:
> >> +{
> >> +    spice_return_if_fail(qxl != NULL);
> >> +    spice_return_if_fail(qxl->st->gl_draw_async == NULL);
> >
> > Here you need to close fd on error or you will get a file
> > descriptor leak.
> 
> >> +
> >> +SPICE_GNUC_VISIBLE
> >> +void spice_gl_draw_async(QXLInstance *qxl,
> >> +                         uint32_t x, uint32_t y,
> >> +                         uint32_t w, uint32_t h,
> >> +                         uint64_t cookie)
> >> +{
> >> +    spice_return_if_fail(qxl != NULL);
> >> +    spice_return_if_fail(qxl->st->scanout.drm_dma_buf_fd != -1);
> >> +    spice_return_if_fail(qxl->st->gl_draw_async == NULL);
> >
> > Here you should signal the cookie (or return an error to be handled by the
> > caller or rendering will stuck
> 
> Those two are pre-conditions, and abort() by default. (even if
> DISABLE_ABORT is set, they should never happen at runtime) Hence it's
> okay to not handle this error case. If you start handling it, then it
> becomes implicit that you deal with those cases and you'll have to
> keep it, but it should never have to in the first place. This is how
> most of glib libraries/applications are written. Since qemu relies on
> glib, they have the same contract with glib libraries, although
> internally they most often use abort() pre-conditions (as spice server
> by default).
> 

Yes, all this is quite messy. 
I don't consider this a stop over anyway.
Actually spice log functions and glib ones are not 100% compatible by
definition and settings.
That's also why it's hard to came to some final decisions.
The spice_return is more of a g_assert but without the possibility of
removing these checks.
The g_return_if is more of an if with logs.
That's actually why spice_assert is more used and understood (as more
similar to g_assert... except the disabling part!)

Frediano