Re: [RFC Patch v4 7/9] correct xc_domain_save()'s return value

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 22/09/14 14:13, Ian Campbell wrote:
> On Mon, 2014-09-22 at 14:03 +0100, Ian Jackson wrote:
>> Ian Campbell writes ("Re: [Xen-devel] [RFC Patch v4 7/9] correct xc_domain_save()'s return value"):
>>> libxc doesn't know that, if it is important then it seems like the
>>> failure + errno ought to be marshalled across the IPC link.
>> Yes, but ...
>>
>>> It may be that this can be easily handled in
>>> libxl__srm_callout_sendreply + helper_getreply. Ian J -- what do you
>>> think?
>> ... while that would be possbile, we have another option.
>>
>> We could say that the callbacks return errno values.  That would
>> simplify the API and avoid having the IPC involve accesses to global
>> variables (ie, things not in the functions' parameter lists).
>>
>> If we do that then it becomes the responsibility of xc_domain_save to
>> either change its own API to return errno, or to save the callback's
>> return value in errno.
> Hrm. libxc is already a complete mess wrt error returning/handling
> because some proportion of the code incorrectly does/assumes this sort
> of thing is happening (because people were confused about the syscall
> returns from the kernel vs. process context). Having a place in libxc
> where this is now done on purpose seems a bit like setting the rope on
> fire to me...
>
> Ian.
>

libxc is an absolute mess, but this is far from the only codepath (even
in xc_domain_save()) which ends up like this.

The *only* safe assumption is that ==0 is success and !=0 is failure for
xc_domain_save(), and errno may or may not be relevant, whether rc is -1
or not.

For the suspend callback itself, I have encountered 3 different error
schemes which stem from a complete lack of expectations set out beside
the function pointer definition in xenguest.h, which is why I had to
copy the old "!=0" check in migration v2.  Even intree in the past, -1
and 1 have been used for errors from this function pointer.


It is my opinion that it is not worth changing any of the error handling
until someone does all of libxc and makes it all consistent.  The risk
for introducing subtle bugs into in (and out-of-) tree callers is just
too high, and this change alone does not fix xc_domain_save() to be
consistent.

~Andrew