[Spice-devel] spice-server, logging and style

[Uri Lublin]

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.