[PATCH 1/4] Use proper header guard for <ufs/ufs.h>

[PATCH 1/4] Use proper header guard for <ufs/ufs.h>

[Marcin Cieslak]

---
 tools/libfsimage/ufs/ufs.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/libfsimage/ufs/ufs.h b/tools/libfsimage/ufs/ufs.h
index 4e7c736..0b178e9 100644
--- a/tools/libfsimage/ufs/ufs.h
+++ b/tools/libfsimage/ufs/ufs.h
@@ -3,7 +3,7 @@
  * Use is subject to license terms.
  */
 
-#ifndef _GRUB_UFS_H
+#ifndef _GRUB_UFS_H_
 #define _GRUB_UFS_H_
 
 /* ufs specific constants */
-- 
2.0.2

[PATCH 2/4] libxl_get_scheduler() cannot return ERROR_FAIL

[Marcin Cieslak]

ERROR_FAIL (-3) is not a proper value for
libxl_scheduler enum.
Use LIBXL_SCHEDULER_UNKNOWN (0) instead.

Clang complains otherwise:

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

C99 does not guarantee that enums are ints (they
can be unsigned).
---
 tools/libxl/libxl.c      | 2 +-
 tools/libxl/xl_cmdimpl.c | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index ad3495a..785a1e7 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -4957,7 +4957,7 @@ libxl_scheduler libxl_get_scheduler(libxl_ctx *ctx)
 
     if ((ret = xc_sched_id(ctx->xch, (int *)&sched)) != 0) {
         LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "getting domain info list");
-        return ERROR_FAIL;
+        return LIBXL_SCHEDULER_UNKNOWN;
     }
     return sched;
 }
diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index 8a38077..86daf8e 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -4821,7 +4821,7 @@ static void output_xeninfo(void)
         return;
     }
 
-    if ((sched = libxl_get_scheduler(ctx)) < 0) {
+    if ((sched = libxl_get_scheduler(ctx)) == LIBXL_SCHEDULER_UNKNOWN) {
         fprintf(stderr, "get_scheduler sysctl failed.\n");
         return;
     }
@@ -6702,7 +6702,7 @@ int main_cpupoolcreate(int argc, char **argv)
             goto out_cfg;
         }
     } else {
-        if ((sched = libxl_get_scheduler(ctx)) < 0) {
+        if ((sched = libxl_get_scheduler(ctx)) == LIBXL_SCHEDULER_UNKNOWN) {
             fprintf(stderr, "get_scheduler sysctl failed.\n");
             goto out_cfg;
         }
-- 
2.0.2

[PATCH 3/4] libxl: shutdown_reason cannot be unsigned

[Marcin Cieslak]

Shutdown reason may be negative, so don't
cast it onto (unsigned int).

Clang complains:

xl_cmdimpl.c:3313:34: error: comparison of unsigned expression >= 0 is always true [-Werror,-Wtautological-compare]
                (shutdown_reason >= 0 &&
                 ~~~~~~~~~~~~~~~ ^  ~
---
 tools/libxl/xl_cmdimpl.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index 86daf8e..f9ca22a 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -3297,7 +3297,7 @@ static void list_domains(int verbose, int context, int claim, int numa,
     printf("\n");
     for (i = 0; i < nb_domain; i++) {
         char *domname;
-        unsigned shutdown_reason;
+        int shutdown_reason;
         domname = libxl_domid_to_name(ctx, info[i].domid);
         shutdown_reason = info[i].shutdown ? info[i].shutdown_reason : 0;
         printf("%-40s %5d %5lu %5d     %c%c%c%c%c%c  %8.1f",
-- 
2.0.2

[PATCH 4/4] libxl: Use yajl_gen_status_ok where appropriate

[Marcin Cieslak]

---
 tools/libxl/libxl_internal.h |  2 +-
 tools/libxl/libxl_json.c     | 10 +++++-----
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 03e9978..f006719 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -1772,7 +1772,7 @@ libxl__json_map_node *libxl__json_map_node_get(const libxl__json_object *o,
 _hidden const libxl__json_object *libxl__json_map_get(const char *key,
                                           const libxl__json_object *o,
                                           libxl__json_node_type expected_type);
-_hidden yajl_status libxl__json_object_to_yajl_gen(libxl__gc *gc_opt,
+_hidden yajl_gen_status libxl__json_object_to_yajl_gen(libxl__gc *gc_opt,
                                                    yajl_gen hand,
                                                    libxl__json_object *param);
 _hidden void libxl__json_object_free(libxl__gc *gc_opt,
diff --git a/tools/libxl/libxl_json.c b/tools/libxl/libxl_json.c
index ceb014a..1481140 100644
--- a/tools/libxl/libxl_json.c
+++ b/tools/libxl/libxl_json.c
@@ -610,12 +610,12 @@ const libxl__json_object *libxl__json_map_get(const char *key,
     return NULL;
 }
 
-yajl_status libxl__json_object_to_yajl_gen(libxl__gc *gc,
+yajl_gen_status libxl__json_object_to_yajl_gen(libxl__gc *gc,
                                            yajl_gen hand,
                                            libxl__json_object *obj)
 {
     int idx = 0;
-    yajl_status rc;
+    yajl_gen_status rc;
 
     switch (obj->type) {
     case JSON_NULL:
@@ -634,17 +634,17 @@ yajl_status libxl__json_object_to_yajl_gen(libxl__gc *gc,
         libxl__json_map_node *node = NULL;
 
         rc = yajl_gen_map_open(hand);
-        if (rc != yajl_status_ok)
+        if (rc != yajl_gen_status_ok)
             return rc;
         for (idx = 0; idx < obj->u.map->count; idx++) {
             if (flexarray_get(obj->u.map, idx, (void**)&node) != 0)
                 break;
 
             rc = libxl__yajl_gen_asciiz(hand, node->map_key);
-            if (rc != yajl_status_ok)
+            if (rc != yajl_gen_status_ok)
                 return rc;
             rc = libxl__json_object_to_yajl_gen(gc, hand, node->obj);
-            if (rc != yajl_status_ok)
+            if (rc != yajl_gen_status_ok)
                 return rc;
         }
         return yajl_gen_map_close(hand);
-- 
2.0.2

Re: [PATCH 1/4] Use proper header guard for <ufs/ufs.h>

[Ian Campbell]

On Sat, 2014-09-13 at 21:10 +0000, Marcin Cieslak wrote:

I didn't spot this on the first patch I commented on but all patches
accepted into Xen need to have a Signed-off-by. Please see
http://wiki.xen.org/wiki/Submitting_Xen_Patches#Signing_off_a_patch.

Ian.

Re: [PATCH 2/4] libxl_get_scheduler() cannot return ERROR_FAIL

[Ian Campbell]

On Sat, 2014-09-13 at 21:10 +0000, Marcin Cieslak wrote:
> ERROR_FAIL (-3) is not a proper value for
> libxl_scheduler enum.
> Use LIBXL_SCHEDULER_UNKNOWN (0) instead.

This came up before, see [0,1] we decided instead that this function
should return an int, I think Julien simply hasn't found the time to
revisit that after the comments on the v2 patch.

[0] http://lists.xen.org/archives/html/xen-devel/2014-03/msg02476.html
[1] http://lists.xen.org/archives/html/xen-devel/2014-03/msg02759.html
> 
> Clang complains otherwise:
> 
> xl_cmdimpl.c:4824:44: error: comparison of unsigned enum expression < 0 is always false [-Werror,-Wtautological-compare]
>     if ((sched = libxl_get_scheduler(ctx)) < 0) {
>         ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ^ ~
> xl_cmdimpl.c:6705:48: error: comparison of unsigned enum expression < 0 is always false [-Werror,-Wtautological-compare]
>         if ((sched = libxl_get_scheduler(ctx)) < 0) {
>             ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ^ ~
> 
> C99 does not guarantee that enums are ints (they
> can be unsigned).
> ---
>  tools/libxl/libxl.c      | 2 +-
>  tools/libxl/xl_cmdimpl.c | 4 ++--
>  2 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> index ad3495a..785a1e7 100644
> --- a/tools/libxl/libxl.c
> +++ b/tools/libxl/libxl.c
> @@ -4957,7 +4957,7 @@ libxl_scheduler libxl_get_scheduler(libxl_ctx *ctx)
>  
>      if ((ret = xc_sched_id(ctx->xch, (int *)&sched)) != 0) {
>          LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "getting domain info list");
> -        return ERROR_FAIL;
> +        return LIBXL_SCHEDULER_UNKNOWN;
>      }
>      return sched;
>  }
> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> index 8a38077..86daf8e 100644
> --- a/tools/libxl/xl_cmdimpl.c
> +++ b/tools/libxl/xl_cmdimpl.c
> @@ -4821,7 +4821,7 @@ static void output_xeninfo(void)
>          return;
>      }
>  
> -    if ((sched = libxl_get_scheduler(ctx)) < 0) {
> +    if ((sched = libxl_get_scheduler(ctx)) == LIBXL_SCHEDULER_UNKNOWN) {
>          fprintf(stderr, "get_scheduler sysctl failed.\n");
>          return;
>      }
> @@ -6702,7 +6702,7 @@ int main_cpupoolcreate(int argc, char **argv)
>              goto out_cfg;
>          }
>      } else {
> -        if ((sched = libxl_get_scheduler(ctx)) < 0) {
> +        if ((sched = libxl_get_scheduler(ctx)) == LIBXL_SCHEDULER_UNKNOWN) {
>              fprintf(stderr, "get_scheduler sysctl failed.\n");
>              goto out_cfg;
>          }

Re: [PATCH 1/4] Use proper header guard for <ufs/ufs.h>

[Ian Campbell]

On Tue, 2014-09-16 at 17:23 +0100, Ian Campbell wrote:
> On Sat, 2014-09-13 at 21:10 +0000, Marcin Cieslak wrote:
> 
> I didn't spot this on the first patch I commented on but all patches
> accepted into Xen need to have a Signed-off-by. Please see
> http://wiki.xen.org/wiki/Submitting_Xen_Patches#Signing_off_a_patch.

Also, please CC the relevant maintainers. You can find these in
MAINTAINERS or use ./scripts/get_maintainer.pl to help automatically
look them up.

Ian.

Re: [PATCH 4/4] libxl: Use yajl_gen_status_ok where appropriate

[Ian Campbell]

On Sat, 2014-09-13 at 21:10 +0000, Marcin Cieslak wrote:

What does "where appropriate" mean here?

Is this compatible with both libjson 1 and 2?

Re: [PATCH 3/4] libxl: shutdown_reason cannot be unsigned

[Julien Grall]

Hello Marcin,

On 13/09/14 14:10, Marcin Cieslak wrote:
> Shutdown reason may be negative, so don't
> cast it onto (unsigned int).

If you are looking the place where the variable is assigned, 
info[i].shutdown_reason is typed as an unsigned int.

So changing type doesn't look like the right solution. I've sent a few 
months ago a patch to drop "shutdown_reason >= 0" check [1].

It was acked, but I hadn't had time to resent this series properly. 
Mainly because I got other errors around with clang.

Regards,

[1] https://patches.linaro.org/27074/



> Clang complains:
>
> xl_cmdimpl.c:3313:34: error: comparison of unsigned expression >= 0 is always true [-Werror,-Wtautological-compare]
>                  (shutdown_reason >= 0 &&
>                   ~~~~~~~~~~~~~~~ ^  ~
> ---
>   tools/libxl/xl_cmdimpl.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> index 86daf8e..f9ca22a 100644
> --- a/tools/libxl/xl_cmdimpl.c
> +++ b/tools/libxl/xl_cmdimpl.c
> @@ -3297,7 +3297,7 @@ static void list_domains(int verbose, int context, int claim, int numa,
>       printf("\n");
>       for (i = 0; i < nb_domain; i++) {
>           char *domname;
> -        unsigned shutdown_reason;
> +        int shutdown_reason;
>           domname = libxl_domid_to_name(ctx, info[i].domid);
>           shutdown_reason = info[i].shutdown ? info[i].shutdown_reason : 0;
>           printf("%-40s %5d %5lu %5d     %c%c%c%c%c%c  %8.1f",
>

-- 
Julien Grall