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
Attachment:
signature.asc
Description: PGP signature
_______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/spice-devel