Re: [PATCH v2] tools/libxl: libxl_get_scheduler should return an int

[Julien Grall <julien.grall@linaro.org>]

On 03/21/2014 02:28 PM, Ian Jackson wrote:
> Julien Grall writes ("[PATCH v2] tools/libxl: libxl_get_scheduler should return an int"):
>> libxl_get_scheduler returns either a valid value in enum range or ERROR_FAIL.
>>
>> As function return type is an enum, chekcing if the value is negative will
>> be always false. Therefore both GCC and clang will never go to the error
>> case.
> ...
> 
> Thanks.
> 
> The libxl part is correct, but I 
> 
>> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
>> index 8990020..7c73ee0 100644
>> --- a/tools/libxl/xl_cmdimpl.c
>> +++ b/tools/libxl/xl_cmdimpl.c
>> @@ -4819,7 +4819,7 @@ int main_vcpuset(int argc, char **argv)
>>  static void output_xeninfo(void)
>>  {
>>      const libxl_version_info *info;
>> -    libxl_scheduler sched;
>> +    int sched;
> 
> OK...
> 
>>      if (!(info = libxl_get_version_info(ctx))) {
>>          fprintf(stderr, "libxl_get_version_info failed.\n");
>> @@ -6706,10 +6706,12 @@ int main_cpupoolcreate(int argc, char **argv)
>>              goto out_cfg;
>>          }
>>      } else {
>> -        if ((sched = libxl_get_scheduler(ctx)) < 0) {
>> +
>> +        if ((ret = libxl_get_scheduler(ctx)) < 0) {
>>              fprintf(stderr, "get_scheduler sysctl failed.\n");
>>              goto out_cfg;
>>          }
>> +        sched = ret;
> 
> But then I don't understand why you changed this too.  Either of these
> changes would suffice by itself, and the former is marginally less
> fiddly.

The variable sched is a libxl_scheduler in this function. I can't modify
the type because it's used with lixbl_scheduler_from_string (see
xl_cmdimpl.c:6703).

If I let sched as an enum I will get the same error as before, e.g

xl_cmdimpl.c:6709:48: error: comparison of unsigned enum expression < 0
is always false [-Werror,-Wtautological-compare]
        if ((sched = libxl_get_scheduler(ctx)) < 0) {
            ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ^ ~

So I need to use ret as a temporary variable.

Regards,

-- 
Julien Grall