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.
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/spice-devel