libxl: build failure due to 'libxl_domain_type'

Introduce LIBXL_DOMAIN_TYPE_INVALID.
Change libxl__domain_type() to return LIBXL_DOMAIN_TYPE_INVALID rather hardcoding -1.
Adjust code pieces where gcc 4.5.3 claims that LIBXL_DOMAIN_TYPE_INVALID is not handled.

This fixes the build error with gcc 4.5.3 reported here:
http://lists.xen.org/archives/html/xen-devel/2012-05/msg01269.html

Quoted 9 lines · Christoph Egger

Introduce LIBXL_DOMAIN_TYPE_INVALID.
Change libxl__domain_type() to return LIBXL_DOMAIN_TYPE_INVALID rather hardcoding -1.
Adjust code pieces where gcc 4.5.3 claims that LIBXL_DOMAIN_TYPE_INVALID is not handled.

This fixes the build error with gcc 4.5.3 reported here:
http://lists.xen.org/archives/html/xen-devel/2012-05/msg01269.html

I was also running into that error and thus I applied this patch, which brought me here:

libxl.c: In function ‘libxl_primary_console_exec’:
libxl.c:1233:14: error: ‘LIBXL_DOMAIN_TYPE_INVALID’ undeclared (first use in this function) libxl.c:1233:14: note: each undeclared identifier is reported only once for each function it appears in

Which actually seems to be right:

$ grep LIBXL_DOMAIN_TYPE_INVALID tools/* -R
tools/libxl/libxl_dm.c:    case LIBXL_DOMAIN_TYPE_INVALID:
tools/libxl/libxl_dm.c:    case LIBXL_DOMAIN_TYPE_INVALID:
tools/libxl/libxl_dom.c:        return LIBXL_DOMAIN_TYPE_INVALID;
tools/libxl/libxl_dom.c:        return LIBXL_DOMAIN_TYPE_INVALID;
tools/libxl/libxl.c:        case LIBXL_DOMAIN_TYPE_INVALID:

Am I missing something?

Quoted 9 lines

Which actually seems to be right:

$ grep LIBXL_DOMAIN_TYPE_INVALID tools/* -R
tools/libxl/libxl_dm.c:    case LIBXL_DOMAIN_TYPE_INVALID:
tools/libxl/libxl_dm.c:    case LIBXL_DOMAIN_TYPE_INVALID:
tools/libxl/libxl_dom.c:        return LIBXL_DOMAIN_TYPE_INVALID;
tools/libxl/libxl_dom.c:        return LIBXL_DOMAIN_TYPE_INVALID;
tools/libxl/libxl.c:        case LIBXL_DOMAIN_TYPE_INVALID:

Quoted 1 line

Am I missing something?

This should be defined in tools/libxl/libxl_types.idl but the patch doesn't seem to add it.

Quoted 9 lines · Ian Campbell

Quoted 9 lines

Which actually seems to be right:

$ grep LIBXL_DOMAIN_TYPE_INVALID tools/* -R
tools/libxl/libxl_dm.c:    case LIBXL_DOMAIN_TYPE_INVALID:
tools/libxl/libxl_dm.c:    case LIBXL_DOMAIN_TYPE_INVALID:
tools/libxl/libxl_dom.c:        return LIBXL_DOMAIN_TYPE_INVALID;
tools/libxl/libxl_dom.c:        return LIBXL_DOMAIN_TYPE_INVALID;
tools/libxl/libxl.c:        case LIBXL_DOMAIN_TYPE_INVALID:

Quoted 5 lines

Quoted 1 line

Am I missing something?

This should be defined in tools/libxl/libxl_types.idl but the patch doesn't seem to add it.

Yep, I'm adding it myself with the attached patch, but I'm now getting this:

_libxl_types.c: In function ‘libxl_domain_build_info_dispose’:
_libxl_types.c:91:5: error: enumeration value ‘LIBXL_DOMAIN_TYPE_INVALID’ not handled in switch [-Werror=switch] _libxl_types.c: In function ‘libxl_domain_build_info_init_type’:
_libxl_types.c:284:5: error: enumeration value ‘LIBXL_DOMAIN_TYPE_INVALID’ not handled in switch [-Werror=switch] testidl.c: In function ‘libxl_domain_build_info_rand_init’:
testidl.c:366:5: error: enumeration value ‘LIBXL_DOMAIN_TYPE_INVALID’ not handled in switch [-Werror=switch] _libxl_types.c: In function ‘libxl_domain_build_info_gen_json’:
_libxl_types.c:1713:5: error: enumeration value ‘LIBXL_DOMAIN_TYPE_INVALID’ not handled in switch [-Werror=switch]
cc1: all warnings being treated as errors

:-O

I wonder if just changing the return type of libxl__domain_type to int would be better than this? I guess it'll probably be much the same.

Quoted 3 lines · Ian Campbell

I wonder if just changing the return type of libxl__domain_type to int would be better than this? I guess it'll probably be much the same.

Well, the patch I'm attaching now let me at least build cleanly, _without_ any other patches (not Christoph's nor mine)... Did you mean something like that?

Quoted 3 lines

Well, the patch I'm attaching now let me at least build cleanly, _without_ any other patches (not Christoph's nor mine)... Did you mean something like that?

I did, I guess we need to check that all callers can cope with this new return value though?

Quoted 4 lines · Ian Campbell

Quoted 3 lines

Well, the patch I'm attaching now let me at least build cleanly, _without_ any other patches (not Christoph's nor mine)... Did you mean something like that?

Quoted 3 lines

I did, I guess we need to check that all callers can cope with this new return value though?

Sure, that was only to be sure I got what you were saying. :-)

What I'm not getting right now is whether or not a proper patch doing such is still interesting or not? Also, how come am I almost the only one seeing that issue? Does it relate to gcc version? :-O

Quoted 8 lines

Quoted 3 lines

I did, I guess we need to check that all callers can cope with this new return value though?

Sure, that was only to be sure I got what you were saying. :-)

What I'm not getting right now is whether or not a proper patch doing such is still interesting or not? Also, how come am I almost the only one seeing that issue? Does it relate to gcc version? :-O

There's been a handful of other reports this week. It does seem to be to do with gcc version, yes.

Quoted 12 lines · Ian Campbell

Quoted 8 lines

Quoted 3 lines

I did, I guess we need to check that all callers can cope with this new return value though?

Sure, that was only to be sure I got what you were saying. :-)

What I'm not getting right now is whether or not a proper patch doing such is still interesting or not? Also, how come am I almost the only one seeing that issue? Does it relate to gcc version? :-O

There's been a handful of other reports this week. It does seem to be to do with gcc version, yes.

Ok then, I didn't notice that. I went through the callers and they seem to be fine with the change, as the return type of the function is pretty much always converted to the enum (i.e., libxl_domain_type) and used in a switch with a proper 'default' clause, in case they care about something different from _HVM or _PV.

So, the below is what I'm using to build (and run) these days... Or was it something different that you meant when saying "check that all callers can cope with this" ?

(I can repost as a separate mail if wanted)

Dario

8<---------------------------

libxl: make libxl__domain_type return 'int'

To avoid gcc > 4.6.3 complaining about:

libxl.c: In function ‘libxl_primary_console_exec’:
libxl.c:1233:9: error: case value ‘4294967295’ not in enumerated type ‘libxl_domain_type’ [-Werror=switch]

Callers have been checked and are fine with the change.

Signed-off-by Dario Faggioli <[email protected]>

diff -r 6dc80df50fa8 tools/libxl/libxl_dom.c

tools/libxl/libxl_dom.c +1 -1

@@ -25,7 +25,7 @@
 
 #include "libxl_internal.h"
 
-libxl_domain_type libxl__domain_type(libxl__gc *gc, uint32_t domid)
+int libxl__domain_type(libxl__gc *gc, uint32_t domid)
 {
     libxl_ctx *ctx = libxl__gc_owner(gc);
     xc_domaininfo_t info;

diff -r 6dc80df50fa8 tools/libxl/libxl_internal.h

tools/libxl/libxl_internal.h +1 -1

@@ -714,7 +714,7 @@ int libxl__self_pipe_eatall(int fd); /* 
 
 
 /* from xl_dom */
-_hidden libxl_domain_type libxl__domain_type(libxl__gc *gc, uint32_t domid);
+_hidden int libxl__domain_type(libxl__gc *gc, uint32_t domid);
 _hidden int libxl__domain_shutdown_reason(libxl__gc *gc, uint32_t domid);
 _hidden int libxl__sched_set_params(libxl__gc *gc, uint32_t domid, libxl_sched_params *scparams);
 #define LIBXL__DOMAIN_IS_TYPE(gc, domid, type) \

Quoted 5 lines

8<---------------------------

libxl: make libxl__domain_type return 'int'

To avoid gcc > 4.6.3 complaining about:

I have gcc 4.5.3 and see this.
Christoph

Quoted 4 lines · Christoph Egger

Quoted 4 lines

8<---------------------------

libxl: make libxl__domain_type return 'int'

Quoted 6 lines

Quoted 1 line

To avoid gcc > 4.6.3 complaining about:

I have gcc 4.5.3 and see this.
Christoph

Ups! You said it earlier but I forgot to go and check your gcc version, and just considered mine. Sorry. :-P

Anyway, let's see what is the fix they want, then we'll decide about a proper changelog. :-)

Thanks and Regards,
Dario

Quoted 28 lines · Dario Faggioli

Quoted 9 lines · Ian Campbell

Quoted 9 lines

Which actually seems to be right:

$ grep LIBXL_DOMAIN_TYPE_INVALID tools/* -R
tools/libxl/libxl_dm.c:    case LIBXL_DOMAIN_TYPE_INVALID:
tools/libxl/libxl_dm.c:    case LIBXL_DOMAIN_TYPE_INVALID:
tools/libxl/libxl_dom.c:        return LIBXL_DOMAIN_TYPE_INVALID;
tools/libxl/libxl_dom.c:        return LIBXL_DOMAIN_TYPE_INVALID;
tools/libxl/libxl.c:        case LIBXL_DOMAIN_TYPE_INVALID:

Quoted 5 lines

Quoted 1 line

Am I missing something?

This should be defined in tools/libxl/libxl_types.idl but the patch doesn't seem to add it.

Yep, I'm adding it myself with the attached patch, but I'm now getting this:

_libxl_types.c: In function ‘libxl_domain_build_info_dispose’:
_libxl_types.c:91:5: error: enumeration value ‘LIBXL_DOMAIN_TYPE_INVALID’ not handled in switch [-Werror=switch] _libxl_types.c: In function ‘libxl_domain_build_info_init_type’:
_libxl_types.c:284:5: error: enumeration value ‘LIBXL_DOMAIN_TYPE_INVALID’ not handled in switch [-Werror=switch] testidl.c: In function ‘libxl_domain_build_info_rand_init’:
testidl.c:366:5: error: enumeration value ‘LIBXL_DOMAIN_TYPE_INVALID’ not handled in switch [-Werror=switch] _libxl_types.c: In function ‘libxl_domain_build_info_gen_json’:
_libxl_types.c:1713:5: error: enumeration value ‘LIBXL_DOMAIN_TYPE_INVALID’ not handled in switch [-Werror=switch]
cc1: all warnings being treated as errors

Quoted 1 line

:-O

I wonder if just changing the return type of libxl__domain_type to int would be better than this? I guess it'll probably be much the same.

Quoted 29 lines · Ian Campbell

Quoted 28 lines · Dario Faggioli

Quoted 9 lines · Ian Campbell

Quoted 9 lines

Which actually seems to be right:

$ grep LIBXL_DOMAIN_TYPE_INVALID tools/* -R
tools/libxl/libxl_dm.c:    case LIBXL_DOMAIN_TYPE_INVALID:
tools/libxl/libxl_dm.c:    case LIBXL_DOMAIN_TYPE_INVALID:
tools/libxl/libxl_dom.c:        return LIBXL_DOMAIN_TYPE_INVALID;
tools/libxl/libxl_dom.c:        return LIBXL_DOMAIN_TYPE_INVALID;
tools/libxl/libxl.c:        case LIBXL_DOMAIN_TYPE_INVALID:

Quoted 5 lines

Quoted 1 line

Am I missing something?

This should be defined in tools/libxl/libxl_types.idl but the patch doesn't seem to add it.

Yep, I'm adding it myself with the attached patch, but I'm now getting this:

_libxl_types.c: In function ‘libxl_domain_build_info_dispose’:
_libxl_types.c:91:5: error: enumeration value ‘LIBXL_DOMAIN_TYPE_INVALID’ not handled in switch [-Werror=switch] _libxl_types.c: In function ‘libxl_domain_build_info_init_type’:
_libxl_types.c:284:5: error: enumeration value ‘LIBXL_DOMAIN_TYPE_INVALID’ not handled in switch [-Werror=switch] testidl.c: In function ‘libxl_domain_build_info_rand_init’:
testidl.c:366:5: error: enumeration value ‘LIBXL_DOMAIN_TYPE_INVALID’ not handled in switch [-Werror=switch] _libxl_types.c: In function ‘libxl_domain_build_info_gen_json’:
_libxl_types.c:1713:5: error: enumeration value ‘LIBXL_DOMAIN_TYPE_INVALID’ not handled in switch [-Werror=switch]
cc1: all warnings being treated as errors

Quoted 4 lines

Quoted 1 line

:-O

I wonder if just changing the return type of libxl__domain_type to int would be better than this? I guess it'll probably be much the same.

Is libxl_domain_type part of the API ?
If yes then it is better to use
'int' and change the enum to #defines to be safe side. An enum used in the API has a backward-compatibility issue related to its size:

An enum is as small as possible to hold the largest value. Whatever 'as small as possible' means depends on the architecture.

Christoph

I wonder if just changing the return type of libxl__domain_type to int would be better than this? I guess it'll probably be much the same.

Quoted 2 lines

_libxl_types.c: In function ‘libxl_domain_build_info_dispose’:
_libxl_types.c:91:5: error: enumeration value ‘LIBXL_DOMAIN_TYPE_INVALID’ not handled in switch [-Werror=switch]

And the upside of making it be an enum is precisely

Quoted 3 lines · Dario Faggioli

Quoted 2 lines

_libxl_types.c: In function ‘libxl_domain_build_info_dispose’:
_libxl_types.c:91:5: error: enumeration value ‘LIBXL_DOMAIN_TYPE_INVALID’ not handled in switch [-Werror=switch]

these warnings, which are alerting us to call sites with broken error handling.

Quoted 6 lines

And the upside of making it be an enum is precisely

Quoted 3 lines · Dario Faggioli

Quoted 2 lines

_libxl_types.c: In function ‘libxl_domain_build_info_dispose’:
_libxl_types.c:91:5: error: enumeration value ‘LIBXL_DOMAIN_TYPE_INVALID’ not handled in switch [-Werror=switch]

Quoted 2 lines

these warnings, which are alerting us to call sites with broken error handling.

I agree, and I can sue try looking at those call sites and see what it takes to add a proper 'case' or 'default' clause in there if you think that to be the way to go...

I think that would be best, if you're willing, thanks.

I would recommend the use of "case" rather than "default" clauses in this case. That way if we introduce a new domain type the compiler will spot all the missing places for us.

Problem arises
with auto-generated code, e.g., in gentypes.py for build_info related functions. In this case, in fact, the libxl_domain_type enum is the key of the keyed-union. For those cases, I was thinking at something like the below:

if isinstance(ty, idl.KeyedUnion):
if parent is None:
raise Exception("KeyedUnion type must have a parent")
s += "switch (%s) {\n" % (parent + ty.keyvar.name) for f in ty.fields:
(nparent,fexpr) = ty.member(v, f, parent is None) s += "case %s:\n" % f.enumname
s += libxl_C_type_dispose(f.type, fexpr, indent + " ", nparent)
s += " break;\n"

+       s += "default:\n    break;\n";
        s += "}\n"

Would it make sense?

Quoted 18 lines · Dario Faggioli

Problem arises
with auto-generated code, e.g., in gentypes.py for build_info related functions. In this case, in fact, the libxl_domain_type enum is the key of the keyed-union. For those cases, I was thinking at something like the below:

if isinstance(ty, idl.KeyedUnion):
if parent is None:
raise Exception("KeyedUnion type must have a parent")
s += "switch (%s) {\n" % (parent + ty.keyvar.name) for f in ty.fields:
(nparent,fexpr) = ty.member(v, f, parent is None) s += "case %s:\n" % f.enumname
s += libxl_C_type_dispose(f.type, fexpr, indent + " ", nparent)
s += " break;\n"

+       s += "default:\n    break;\n";
        s += "}\n"

Quoted 2 lines

Would it make sense?

Like this thing below.

Christoph, this ended up extending what you sent at the very beginning of this thread, so I think we should both sign-off-by it (and thus it took the liberty going ahead and adding yours), do you agree?

<-----------------------

libxl: introduce LIBXL_DOMAIN_TYPE_INVALID

To avoid recent gcc complaining about:
libxl.c: In function ‘libxl_primary_console_exec’:
libxl.c:1233:9: error: case value ‘4294967295’ not in enumerated type ‘libxl_domain_type’ [-Werror=switch]

Adjust code pieces where -Wswitch makes it claim that LIBXL_DOMAIN_TYPE_INVALID is not handled.

Signed-off-by Dario Faggioli <[email protected]>

Signed-off-by Christoph Egger <[email protected]>

diff -r 6dc80df50fa8 tools/libxl/gentest.py

tools/libxl/gentest.py +2 -0

@@ -37,6 +37,8 @@ def gen_rand_init(ty, v, indent = "    "
             s += "case %s:\n" % f.enumname
             s += gen_rand_init(f.type, fexpr, indent + "    ", nparent)
             s += "    break;\n"
+        s += "default:\n";
+        s += "    break;\n";
         s += "}\n"
     elif isinstance(ty, idl.Struct) \
      and (parent is None or ty.json_fn is None):

diff -r 6dc80df50fa8 tools/libxl/gentypes.py

tools/libxl/gentypes.py +6 -0

@@ -65,6 +65,8 @@ def libxl_C_type_dispose(ty, v, indent =
             s += "case %s:\n" % f.enumname
             s += libxl_C_type_dispose(f.type, fexpr, indent + "    ", nparent)
             s += "    break;\n"
+        s += "default:\n";
+        s += "    break;\n";
         s += "}\n"
     elif isinstance(ty, idl.Struct) and (parent is None or ty.dispose_fn is None):
         for f in [f for f in ty.fields if not f.const]:

@@ -98,6 +100,8 @@ def _libxl_C_type_init(ty, v, indent = "
                 s += "case %s:\n" % f.enumname
                 s += _libxl_C_type_init(f.type, fexpr, "    ", nparent)
                 s += "    break;\n"
+            s += "default:\n";
+            s += "    break;\n";
             s += "}\n"
         else:
             if ty.keyvar.init_val:

@@ -177,6 +181,8 @@ def libxl_C_type_gen_json(ty, v, indent 
             s += "case %s:\n" % f.enumname
             s += libxl_C_type_gen_json(f.type, fexpr, indent + "    ", nparent)
             s += "    break;\n"
+        s += "default:\n";
+        s += "    break;\n";
         s += "}\n"
     elif isinstance(ty, idl.Struct) and (parent is None or ty.json_fn is None):
         s += "s = yajl_gen_map_open(hand);\n"

diff -r 6dc80df50fa8 tools/libxl/libxl.c

tools/libxl/libxl.c +1 -1

@@ -1230,7 +1230,7 @@ int libxl_primary_console_exec(libxl_ctx
         case LIBXL_DOMAIN_TYPE_PV:
             rc = libxl_console_exec(ctx, domid_vm, 0, LIBXL_CONSOLE_TYPE_PV);
             break;
-        case -1:
+        case LIBXL_DOMAIN_TYPE_INVALID:
             LOG(ERROR,"unable to get domain type for domid=%"PRIu32,domid_vm);
             rc = ERROR_FAIL;
             break;

diff -r 6dc80df50fa8 tools/libxl/libxl_dm.c

tools/libxl/libxl_dm.c +4 -0

@@ -257,6 +257,8 @@ static char ** libxl__build_device_model
         for (i = 0; b_info->extra_hvm && b_info->extra_hvm[i] != NULL; i++)
             flexarray_append(dm_args, b_info->extra_hvm[i]);
         break;
+    case LIBXL_DOMAIN_TYPE_INVALID:
+        break;
     }
     flexarray_append(dm_args, NULL);
     return (char **) flexarray_contents(dm_args);

@@ -505,6 +507,8 @@ static char ** libxl__build_device_model
         for (i = 0; b_info->extra_hvm && b_info->extra_hvm[i] != NULL; i++)
             flexarray_append(dm_args, b_info->extra_hvm[i]);
         break;
+    case LIBXL_DOMAIN_TYPE_INVALID:
+        break;
     }
 
     ram_size = libxl__sizekb_to_mb(b_info->max_memkb - b_info->video_memkb);

diff -r 6dc80df50fa8 tools/libxl/libxl_dom.c

tools/libxl/libxl_dom.c +2 -2

@@ -33,9 +33,9 @@ libxl_domain_type libxl__domain_type(lib
 
     ret = xc_domain_getinfolist(ctx->xch, domid, 1, &info);
     if (ret != 1)
-        return -1;
+        return LIBXL_DOMAIN_TYPE_INVALID;
     if (info.domain != domid)
-        return -1;
+        return LIBXL_DOMAIN_TYPE_INVALID;
     if (info.flags & XEN_DOMINF_hvm_guest)
         return LIBXL_DOMAIN_TYPE_HVM;
     else

diff -r 6dc80df50fa8 tools/libxl/libxl_types.idl

tools/libxl/libxl_types.idl +1 -0

@@ -30,6 +30,7 @@ MemKB = UInt(64, init_val = "LIBXL_MEMKB
 #
 
 libxl_domain_type = Enumeration("domain_type", [
+    (-1, "INVALID"),
     (1, "HVM"),
     (2, "PV"),
     ])

That's what I'm doing for any explicit usage of the enum. Problem arises with auto-generated code, e.g., in gentypes.py for build_info related functions. In this case, in fact, the libxl_domain_type enum is the key of the keyed-union. For those cases, I was thinking at something like the below:

if isinstance(ty, idl.KeyedUnion):
if parent is None:
raise Exception("KeyedUnion type must have a parent")
s += "switch (%s) {\n" % (parent + ty.keyvar.name) for f in ty.fields:
(nparent,fexpr) = ty.member(v, f, parent is None) s += "case %s:\n" % f.enumname
s += libxl_C_type_dispose(f.type, fexpr, indent + " ", nparent)
s += " break;\n"

+       s += "default:\n    break;\n";
        s += "}\n"

Would it make sense?

Quoted 12 lines

if isinstance(ty, idl.KeyedUnion):
if parent is None:
raise Exception("KeyedUnion type must have a parent")
s += "switch (%s) {\n" % (parent + ty.keyvar.name) for f in ty.fields:
(nparent,fexpr) = ty.member(v, f, parent is None) s += "case %s:\n" % f.enumname
s += libxl_C_type_dispose(f.type, fexpr, indent + " ", nparent)
s += " break;\n"

+       s += "default:\n    break;\n";
        s += "}\n"

Quoted 1 line

Would it make sense?

No, I don't think so. Surely the idl should contain an explicitly empty structure ?