[Spice-devel] [PATCH] Avoid NULL-dereference if canvas_get_image errors out

[Christophe Fergeau]

On Tue, Jul 03, 2018 at 04:31:10PM +0200, Christophe de Dinechin wrote:
> I think we never had a discussion of what we really want in each case, and that’s causing the confusion.
> 
> First, a meta-rule. Like you, there is a lot in SPICE code I don’t
> like. When in doubt, I try to use consistency as a guide. If the code
> uses spice_critical in that file, my changes will use spice_critical.
> That’s the case for canvas_base.c.

There are 2 occurrences of spice_critical in canvas_base.c, I'd push for
changing these 2, and really discourage use of
spice_critical/spice_warning in new code.

> 
> Then, on the specific point you raised. As I understand it,
> spice_critical *may* abort, but it does not always abort, it depends
> on the abort flag. Therefore, it is correct to free the resource in
> case it returns. Do you disagree with any part of that rationale (i.e.
> either disagree that spice_critical may return, or that we should free
> if it does)?

spice_critical by default will abort(), it used to return in spice-gtk
and abort() in spice-server, but this is no longer the case,
spice_critical aborts in both cases. You could prevent spice_critical
from aborting if you set SPICE_ABORT_LEVEL, but I don't think we should
support someone reporting a bug after changing the default behaviour of
spice_critical(). So for me spice_critical() aborts(). If that's what
you want, I would not bother with the cleanups. If that's not what you
want, I'd use g_critical(), g_warning(), spice_warning() instead, and do
the cleanup.

Christophe
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/spice-devel/attachments/20180703/3fbfa49a/attachment.sig>