[PATCH] tools/libxl: Correctly check if libxl_get_scheduler has failed

[PATCH] tools/libxl: Correctly check if libxl_get_scheduler has failed

[Julien Grall]

libxl_get_scheduler will return an enum, therefore checking if the value
is negative is wrong.

Both GCC and clang will never go to the error case.

Spotted by clang:
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) {
            ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ^ ~

Signed-off-by: Julien Grall <julien.grall@linaro.org>

---
    I'm not sure this is the right way to test if libxl_get_scheduler has
    failed.

    This small program should print ERROR, but on both clang and gcc it will
    print OK.
 #include <stdio.h>

typedef enum libxl_error
{
    ERROR_FAIL = -3,
} libxl_error;

typedef enum libxl_sched
{
  SCHED_SCHED,
} libxl_sched;

libxl_sched f(void)
{
    return ERROR_FAIL;
}

int main(void)
{
    printf("f() = %d\n", f());
    if ( f() < 0 )
        printf("ERROR\n");
    else
        printf("OK\n");

    return 0;
}
---
 tools/libxl/xl_cmdimpl.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index 8990020..8f6c411 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -4826,7 +4826,7 @@ static void output_xeninfo(void)
         return;
     }
 
-    if ((sched = libxl_get_scheduler(ctx)) < 0) {
+    if ((int)(sched = libxl_get_scheduler(ctx)) < 0) {
         fprintf(stderr, "get_scheduler sysctl failed.\n");
         return;
     }
@@ -6706,7 +6706,7 @@ int main_cpupoolcreate(int argc, char **argv)
             goto out_cfg;
         }
     } else {
-        if ((sched = libxl_get_scheduler(ctx)) < 0) {
+        if ((int)(sched = libxl_get_scheduler(ctx)) < 0) {
             fprintf(stderr, "get_scheduler sysctl failed.\n");
             goto out_cfg;
         }
-- 
1.7.10.4

Re: [PATCH] tools/libxl: Correctly check if libxl_get_scheduler has failed

[Ian Jackson]

Julien Grall writes ("[PATCH] tools/libxl: Correctly check if libxl_get_scheduler has failed"):
> libxl_get_scheduler will return an enum, therefore checking if the value
> is negative is wrong.

Gah.  enums in C are so hateful.

> -    if ((sched = libxl_get_scheduler(ctx)) < 0) {
> +    if ((int)(sched = libxl_get_scheduler(ctx)) < 0) {

I'm afraid this isn't the right fix.  The right fix is not to use the
enum type at all.  libxl_get_scheduler, and the "sched" local, should
be ints.

Ian.

Re: [PATCH] tools/libxl: Correctly check if libxl_get_scheduler has failed

[Julien Grall]

Hi Ian,

On 03/20/2014 01:23 PM, Ian Jackson wrote:
> Julien Grall writes ("[PATCH] tools/libxl: Correctly check if libxl_get_scheduler has failed"):
>> libxl_get_scheduler will return an enum, therefore checking if the value
>> is negative is wrong.
> 
> Gah.  enums in C are so hateful.
> 
>> -    if ((sched = libxl_get_scheduler(ctx)) < 0) {
>> +    if ((int)(sched = libxl_get_scheduler(ctx)) < 0) {
> 
> I'm afraid this isn't the right fix.  The right fix is not to use the
> enum type at all.  libxl_get_scheduler, and the "sched" local, should
> be ints.

Thanks. I will rework the patch.

Regards,

-- 
Julien Grall