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

[Uri Lublin]

On 11/16/2015 01:20 PM, Frediano Ziglio wrote:
>>
>> 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.

I kind of think of qemu-kvm as internal (the interfaces not e.g traffic
coming from the guest through it).

>
> On specific implementation I would like to remember that actually
> spice_return_if* macros by default abort too.

Right, I think that's better change -- abort only when
testing/debugging.

Uri.