Re: [Xen-devel] [PATCH v4] libxc: check for various libxc failures and pass them down

[Matthew Daley <mattjd@xxxxxxxxx>]

On Tue, Nov 5, 2013 at 6:31 AM, Ian Campbell <Ian.Campbell@xxxxxxxxxx> wrote:
> On Sun, 2013-11-03 at 12:08 +1300, Matthew Daley wrote:
>>  static int xc_cpuid_policy(
>>      xc_interface *xch, domid_t domid,
>>      const unsigned int *input, unsigned int *regs)
>>  {
>> +    int                 rc;
>>      xc_dominfo_t        info;
>>
>> -    if ( xc_domain_getinfo(xch, domid, 1, &info) == 0 )
>> +    rc = xc_domain_getinfo(xch, domid, 1, &info);
>> +    if ( rc < 0 )
>> +        return rc;
>> +    if ( rc == 0 )
>>          return -EINVAL;
>
> I think this means that this function now returns a mixture of -1 (with
> errno set) and -errno depending on the failure mode. libxc really is a
> horrible chuffing mess in this regard, sorry.

I noticed :)

>
> Luckily nothing right now looks at the return value (you are adding
> those checks to the caller in this patch) so I think you can just
> convert this into { errno=EINVAL; return -1; }
>
> Then again, it looks like xc_cpuid_set (the aforementioned called)
> already returns both styles (explicit rc = -EINVAL, vs returning the
> result of do_domctl which is -1 and set errno). Gah!
>
> Hah, but no one looks at the specific value of the result of
> xc_cpuid_set on failure: libxl doesn't check it at all, Python and ocaml
> bindings turn it into a generic exception using xc_get_last_error.
>
> WTF, xc_get_last_error is a *third* error handling mechanism in libxc,
> apparently built into the logging infrastructure. Which isn't used at
> all on this code path AFAICT.
>
> What a terrible mess. I think I should just apply this patch and putthe
> rug back over the rest of it :-(

Perhaps this patch should be dropped and the issues put in a "take a
proper look at later" pile?

- Matthew

>
> Ian.
>
>

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel