> > On 11/03/2015 05:11 PM, Christophe Fergeau wrote: > > Hey, > > > > On Tue, Nov 03, 2015 at 08:15:14AM -0500, Frediano Ziglio wrote: > >> Style: > >> > >> Personal considerations: > >> - it seems some changes in patchset change "if (cond) return" code with > >> spice_return_if_fail(cond). As stated before the macro are very > >> misleading > >> and the result is usually a program abortion instead of a return. > >> I think new macro with better naming (I don't have a specific > >> suggestion) > >> would be better. The code is actually less readable with these macros; > > > > Regarding this first point, I'm personally fine with the naming (or > > using g_return_if_fail() instead) as long as it's clear that they have > > the same meaning as in glib: > > > > "If expr evaluates to FALSE, the current function should be considered > > to have undefined behaviour (a programmer error). The only correct > > solution to such an error is to change the module that is calling the > > current function, so that it avoids this incorrect call. > > > > To make this undefined behaviour visible, if expr evaluates to FALSE, > > the result is usually that a critical message is logged and the current > > function returns." > > I think we should distinguish between internal-errors (spice-server > and maybe also qemu-kvm) and external-errors (spice-client and guest). > > For internal-errors, an unexpected behavior is indeed very likely > due to a bug. We should assert. > > For external-errors, an unexpected behavior can be due to a buggy > (or malicious) client/guest. Such cases are not to be considered > a bug (of spice-server) and spice-server should not abort. > > For example, assume spice-server receives a message it does not know > from the client. It would be better to ignore such a message > (or send an error back to the client or disconnect the client...) > than to abort. > > When testing it's better to abort on any error (expect for > tests that tests handling of a specific error). > > Regards, > Uri. > > I like this distinction between internal and external. I agree internal should assert as a code bug was found. Also agree for external we should not abort but handle the condition. I would put also an exception depending on the external source. If Qemu is calling an API in an unexpected way (say for instance passing a NULL pointer for a structure to initialize for instance) I would define this a bug even if the source is external. Perhaps considering this would be better to define a secure source and insecure one and abort on secure while handle the insecure. On specific implementation I would like to remember that actually spice_return_if* macros by default abort too. Frediano _______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/spice-devel