> On 3 Jul 2018, at 16:58, Christophe Fergeau <cfergeau@xxxxxxxxxx> wrote: > > 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. Again, agree to change globally. Disagree with you to connect the discussion with this patch, and disagree to create a local inconsistency for just a single line added. > >> >> 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(). As long as there is a case (even if it’s not the default behaviour and infrequent) where spice_critical() returns, suggesting to *remove* the cleanup is incorrect. It’s not a matter of “not supporting”, it’s a matter of you requesting me to actually go ahead and change my code to break a case that is not broken in the patch as I submitted it. > So for me spice_critical() aborts(). No, as stated above, and as stated by me earlier, spice_critical() *may* abort, but it may also *return*. That’s sufficient ground to leave the cleanup code in place. > If that's what > you want, I would not bother with the cleanups. I want spice_ctitical because that’s what is being used in the file today (irrespective of what I may otherwise think about the incredibly confusing mess of SPICE and glib logging in general). And since spice_critical() may return, I need to leave the cleanup in place at the moment. > If that's not what you > want, I'd use g_critical(), g_warning(), spice_warning() instead, and do > the cleanup. I fail to see how g_critical is any different. It also may return or exit the program depending on some external environment. Only the default is different, but I do not write code to deal only with the default case, otherwise this patch would not even exist (canvas_get_image does not return NULL by default). Thanks Christophe > > Christophe > _______________________________________________ > Spice-devel mailing list > Spice-devel@xxxxxxxxxxxxxxxxxxxxx > https://lists.freedesktop.org/mailman/listinfo/spice-devel _______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/spice-devel