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

[Frediano Ziglio]

> 
> 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