Dario Faggioli
2012-May-24 15:37 UTC
[PATCH v4] 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 <dario.faggioli@citrix.com>
Signed-off-by: Christoph Egger <Christoph.Egger@amd.com>
diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -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 --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
--- a/tools/libxl/libxl_dm.c
+++ b/tools/libxl/libxl_dm.c
@@ -257,6 +257,10 @@ 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:
+ LIBXL__LOG(CTX, LIBXL__LOG_ERROR, "invalid domain type");
+ flexarray_free(dm_args);
+ return NULL;
}
flexarray_append(dm_args, NULL);
return (char **) flexarray_contents(dm_args);
@@ -505,6 +509,10 @@ 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:
+ LIBXL__LOG(CTX, LIBXL__LOG_ERROR, "invalid domain type");
+ flexarray_free(dm_args);
+ return NULL;
}
ram_size = libxl__sizekb_to_mb(b_info->max_memkb -
b_info->video_memkb);
diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
--- a/tools/libxl/libxl_dom.c
+++ b/tools/libxl/libxl_dom.c
@@ -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 --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
--- a/tools/libxl/libxl_types.idl
+++ b/tools/libxl/libxl_types.idl
@@ -30,6 +30,7 @@ MemKB = UInt(64, init_val = "LIBXL_MEMKB
#
libxl_domain_type = Enumeration("domain_type", [
+ (-1, "INVALID"),
(1, "HVM"),
(2, "PV"),
])
@@ -315,6 +316,7 @@ libxl_domain_build_info = Struct("domain
# Use host's E820 for PCI
passthrough.
("e820_host", libxl_defbool),
])),
+ ("invalid", Struct(None, [])),
], keyvar_init_val = "-1")),
], dir=DIR_IN
)
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Ian Campbell
2012-May-25 09:44 UTC
Re: [PATCH v4] libxl: introduce LIBXL_DOMAIN_TYPE_INVALID
On Thu, 2012-05-24 at 16:37 +0100, Dario Faggioli wrote:> 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 <dario.faggioli@citrix.com> > Signed-off-by: Christoph Egger <Christoph.Egger@amd.com> > > diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c > --- a/tools/libxl/libxl.c > +++ b/tools/libxl/libxl.c > @@ -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 --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c > --- a/tools/libxl/libxl_dm.c > +++ b/tools/libxl/libxl_dm.c > @@ -257,6 +257,10 @@ 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: > + LIBXL__LOG(CTX, LIBXL__LOG_ERROR, "invalid domain type"); > + flexarray_free(dm_args); > + return NULL;In some cases, like here, the code is entitled to assume that type is either PV or HVM, due to the checks in libxl__domain_build_info_setdefault. I think if we see an invalid here then that is an abort() worthy event. There are a bunch of places where we look a b_info->type with if statements instead of switch. Plain ifs are ok, but it might be worth checking the ones with an else clause (not else if) too? I suspect in many cases they are entitled that !HVM == PV due to the setdefault thing.> } > flexarray_append(dm_args, NULL); > return (char **) flexarray_contents(dm_args); > @@ -505,6 +509,10 @@ 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: > + LIBXL__LOG(CTX, LIBXL__LOG_ERROR, "invalid domain type"); > + flexarray_free(dm_args); > + return NULL;abort() here too.> } > > ram_size = libxl__sizekb_to_mb(b_info->max_memkb - b_info->video_memkb); > diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c > --- a/tools/libxl/libxl_dom.c > +++ b/tools/libxl/libxl_dom.c > @@ -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 --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl > --- a/tools/libxl/libxl_types.idl > +++ b/tools/libxl/libxl_types.idl > @@ -30,6 +30,7 @@ MemKB = UInt(64, init_val = "LIBXL_MEMKB > # > > libxl_domain_type = Enumeration("domain_type", [ > + (-1, "INVALID"), > (1, "HVM"), > (2, "PV"), > ]) > @@ -315,6 +316,7 @@ libxl_domain_build_info = Struct("domain > # Use host's E820 for PCI passthrough. > ("e820_host", libxl_defbool), > ])), > + ("invalid", Struct(None, [])), > ], keyvar_init_val = "-1")), > ], dir=DIR_IN > )_______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Dario Faggioli
2012-May-25 10:26 UTC
Re: [PATCH v4] libxl: introduce LIBXL_DOMAIN_TYPE_INVALID
On Fri, 2012-05-25 at 10:44 +0100, Ian Campbell wrote:> > diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c > > --- a/tools/libxl/libxl_dm.c > > +++ b/tools/libxl/libxl_dm.c > > @@ -257,6 +257,10 @@ 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: > > + LIBXL__LOG(CTX, LIBXL__LOG_ERROR, "invalid domain type"); > > + flexarray_free(dm_args); > > + return NULL; > > In some cases, like here, the code is entitled to assume that type is > either PV or HVM, due to the checks in > libxl__domain_build_info_setdefault. I think if we see an invalid here > then that is an abort() worthy event. >As you wish, although it seems to me that this case above is the only possible situation where we are returning NULL from that function. Nevertheless, a NULL return value is handled and considered (and propagated) as error by the caller. That''s why, when Roger suggested going this way (i.e., retuning NULL), I liked the idea very much and went for it. That being said, I''m very very very few confident with these code paths, so I''m just thought it might worth pointing the above out, but I''ll definitely cope with your request if you really thing this is they correct thing o do.> There are a bunch of places where we look a b_info->type with if > statements instead of switch. Plain ifs are ok, but it might be worth > checking the ones with an else clause (not else if) too? I suspect in > many cases they are entitled that !HVM == PV due to the setdefault > thing. >Right, and many of them I can take care of. Perhaps the problem is there are some places where the event of libxl__domain_type "failing" (i.e., returning something different from HVM or PV) is somewhat considered impossible, or at least neglected, like this one here: int libxl_domain_suspend(libxl_ctx *ctx, libxl_domain_suspend_info *info, uint32_t domid, int fd) { GC_INIT(ctx); libxl_domain_type type = libxl__domain_type(gc, domid); int live = info != NULL && info->flags & XL_SUSPEND_LIVE; int debug = info != NULL && info->flags & XL_SUSPEND_DEBUG; int rc = 0; rc = libxl__domain_suspend_common(gc, domid, fd, type, live, debug, /* No Remus */ NULL); if (!rc && type == LIBXL_DOMAIN_TYPE_HVM) rc = libxl__domain_save_device_model(gc, domid, fd); GC_FREE; return rc; } Of course that can be right because of your argument about _sefdefault, but I''m not sure how to make sure of that and what to do about them... Thoughts? On a related note, re what we where discussing yesterday on IRC about putting a ''default'' clause on those switches, there already is some heterogeneity here. For example: int libxl_domain_destroy(libxl_ctx *ctx, uint32_t domid) { ... switch (libxl__domain_type(gc, domid)) { case LIBXL_DOMAIN_TYPE_HVM: ... break; case LIBXL_DOMAIN_TYPE_PV: ... break; default: abort(); } ... } or: int libxl_primary_console_exec(libxl_ctx *ctx, uint32_t domid_vm) { ... else { switch (libxl__domain_type(gc, domid_vm)) { case LIBXL_DOMAIN_TYPE_HVM: ... break; case LIBXL_DOMAIN_TYPE_PV: ... break; case LIBXL_DOMAIN_TYPE_INVALID: ... break; default: abort(); } ... } Should we leave it like this? Is it worth/reasonable to convert all the ''default:'' into ''case LIBXL_DOMAIN_TYPE_INVALID:'' (if yes, what do we do with the places that have both? :O) ? Or perhaps vice versa? If we can gather consensus on an alternative, I can go ahead and put it down all over the places... Thanks and Regards, Dario -- <<This happens because I choose it to happen!>> (Raistlin Majere) ----------------------------------------------------------------- Dario Faggioli, Ph.D, http://retis.sssup.it/people/faggioli Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK) _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Ian Campbell
2012-May-25 10:36 UTC
Re: [PATCH v4] libxl: introduce LIBXL_DOMAIN_TYPE_INVALID
On Fri, 2012-05-25 at 11:26 +0100, Dario Faggioli wrote:> On Fri, 2012-05-25 at 10:44 +0100, Ian Campbell wrote: > > > diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c > > > --- a/tools/libxl/libxl_dm.c > > > +++ b/tools/libxl/libxl_dm.c > > > @@ -257,6 +257,10 @@ 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: > > > + LIBXL__LOG(CTX, LIBXL__LOG_ERROR, "invalid domain type"); > > > + flexarray_free(dm_args); > > > + return NULL; > > > > In some cases, like here, the code is entitled to assume that type is > > either PV or HVM, due to the checks in > > libxl__domain_build_info_setdefault. I think if we see an invalid here > > then that is an abort() worthy event. > > > As you wish, although it seems to me that this case above is the only > possible situation where we are returning NULL from that function. > Nevertheless, a NULL return value is handled and considered (and > propagated) as error by the caller. That''s why, when Roger suggested > going this way (i.e., retuning NULL), I liked the idea very much and > went for it. > > That being said, I''m very very very few confident with these code paths, > so I''m just thought it might worth pointing the above out, but I''ll > definitely cope with your request if you really thing this is they > correct thing o do.It would be a bug, similar to an assertion failure, to get to this point with b_info->type not equal to either PV or HVM. This comment about setdefault in libxl_internal.h explains why: * Idempotently sets any members of "p" which is currently set to * a special value indicating that the defaults should be used * (per libxl_<type>_init) to a specific value. * * All libxl API functions are expected to have arranged for this * to be called before using any values within these structures. so having arranged to call that function at the right time we can assume that type is a sensible value, and indeed setdefault makes this the case.> > There are a bunch of places where we look a b_info->type with if > > statements instead of switch. Plain ifs are ok, but it might be worth > > checking the ones with an else clause (not else if) too? I suspect in > > many cases they are entitled that !HVM == PV due to the setdefault > > thing. > > > > Right, and many of them I can take care of. Perhaps the problem is there > are some places where the event of libxl__domain_type "failing" (i.e., > returning something different from HVM or PV) is somewhat considered > impossible, or at least neglected, like this one here: > > int libxl_domain_suspend(libxl_ctx *ctx, libxl_domain_suspend_info *info, > uint32_t domid, int fd) > { > GC_INIT(ctx); > libxl_domain_type type = libxl__domain_type(gc, domid); > int live = info != NULL && info->flags & XL_SUSPEND_LIVE; > int debug = info != NULL && info->flags & XL_SUSPEND_DEBUG; > int rc = 0; > > rc = libxl__domain_suspend_common(gc, domid, fd, type, live, debug, > /* No Remus */ NULL); > > if (!rc && type == LIBXL_DOMAIN_TYPE_HVM) > rc = libxl__domain_save_device_model(gc, domid, fd); > GC_FREE; > return rc; > } > > > Of course that can be right because of your argument about _sefdefault, > but I''m not sure how to make sure of that and what to do about them... > Thoughts?This one is OK, it doesn''t have an else case...> > > On a related note, re what we where discussing yesterday on IRC about > putting a ''default'' clause on those switches, there already is some > heterogeneity here. For example: > > > int libxl_domain_destroy(libxl_ctx *ctx, uint32_t domid) > { > ... > switch (libxl__domain_type(gc, domid)) { > case LIBXL_DOMAIN_TYPE_HVM: > ... > break; > case LIBXL_DOMAIN_TYPE_PV: > ... > break; > default: > abort();This seems like it needs a TYPE_INVALID, since libxl__domain_type could legitimately return it.> } > ... > } > > or: > > int libxl_primary_console_exec(libxl_ctx *ctx, uint32_t domid_vm) > { > ... > else { > switch (libxl__domain_type(gc, domid_vm)) { > case LIBXL_DOMAIN_TYPE_HVM: > ... > break; > case LIBXL_DOMAIN_TYPE_PV: > ... > break; > case LIBXL_DOMAIN_TYPE_INVALID: > ... > break; > default: > abort(); > } > ... > } > > Should we leave it like this? Is it worth/reasonable to convert all the > ''default:'' into ''case LIBXL_DOMAIN_TYPE_INVALID:'' (if yes, what do we do > with the places that have both? :O) ? Or perhaps vice versa? > > If we can gather consensus on an alternative, I can go ahead and put it > down all over the places...> > Thanks and Regards, > Dario >
Ian Jackson
2012-May-25 11:03 UTC
Re: [PATCH v4] libxl: introduce LIBXL_DOMAIN_TYPE_INVALID
Ian Campbell writes ("Re: [PATCH v4] libxl: introduce
LIBXL_DOMAIN_TYPE_INVALID"):> so having arranged to call that function at the right time we can assume
> that type is a sensible value, and indeed setdefault makes this the
> case.
Right.
The other situation where we can get _INVALID is if libxl__domain_type
fails, which it can do.
I think this should be handled by having places which call
libxl__domain_type abandon operation and return an error if the
libxl__domain_type fails.
If this is done, then general variables, parameters, etc. within libxl
which are supposed to contain a libxl_domain_type will never contain
_INVALID.
Ian.
Dario Faggioli
2012-May-25 13:10 UTC
Re: [PATCH v4] libxl: introduce LIBXL_DOMAIN_TYPE_INVALID
On Fri, 2012-05-25 at 12:03 +0100, Ian Jackson wrote:> Ian Campbell writes ("Re: [PATCH v4] libxl: introduce LIBXL_DOMAIN_TYPE_INVALID"): > > so having arranged to call that function at the right time we can assume > > that type is a sensible value, and indeed setdefault makes this the > > case. > > Right. >Ok.> The other situation where we can get _INVALID is if libxl__domain_type > fails, which it can do. > > I think this should be handled by having places which call > libxl__domain_type abandon operation and return an error if the > libxl__domain_type fails. > > If this is done, then general variables, parameters, etc. within libxl > which are supposed to contain a libxl_domain_type will never contain > _INVALID. >I like this. I''ll chase each call to that function and have the calle failing if a DOMAIN_TYPE_INVALID is returned. Then, if I go this way, can I also nuke both the ''case DOMAIN_TYPE_INVALID'' _and_ the default clauses from everywhere? I seem to think I could... Thanks and Regards, Daio -- <<This happens because I choose it to happen!>> (Raistlin Majere) ----------------------------------------------------------------- Dario Faggioli, Ph.D, http://retis.sssup.it/people/faggioli Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK) _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Ian Campbell
2012-May-25 14:00 UTC
Re: [PATCH v4] libxl: introduce LIBXL_DOMAIN_TYPE_INVALID
On Fri, 2012-05-25 at 14:10 +0100, Dario Faggioli wrote:> On Fri, 2012-05-25 at 12:03 +0100, Ian Jackson wrote: > > Ian Campbell writes ("Re: [PATCH v4] libxl: introduce LIBXL_DOMAIN_TYPE_INVALID"): > > > so having arranged to call that function at the right time we can assume > > > that type is a sensible value, and indeed setdefault makes this the > > > case. > > > > Right. > > > Ok. > > > The other situation where we can get _INVALID is if libxl__domain_type > > fails, which it can do. > > > > I think this should be handled by having places which call > > libxl__domain_type abandon operation and return an error if the > > libxl__domain_type fails. > > > > If this is done, then general variables, parameters, etc. within libxl > > which are supposed to contain a libxl_domain_type will never contain > > _INVALID. > > > I like this. I''ll chase each call to that function and have the calle > failing if a DOMAIN_TYPE_INVALID is returned. Then, if I go this way, > can I also nuke both the ''case DOMAIN_TYPE_INVALID'' _and_ the default > clauses from everywhere? I seem to think I could...iff the compiler is smart enough to realise that in the type == INVALID case you have returned already before reaching the switch statement, otherwise you will need to have "case INVALID: abort()".> > Thanks and Regards, > Daio >
Christoph Egger
2012-Jun-04 13:11 UTC
Re: [PATCH v4] libxl: introduce LIBXL_DOMAIN_TYPE_INVALID
On 05/25/12 16:00, Ian Campbell wrote:> On Fri, 2012-05-25 at 14:10 +0100, Dario Faggioli wrote: >> On Fri, 2012-05-25 at 12:03 +0100, Ian Jackson wrote: >>> Ian Campbell writes ("Re: [PATCH v4] libxl: introduce LIBXL_DOMAIN_TYPE_INVALID"): >>>> so having arranged to call that function at the right time we can assume >>>> that type is a sensible value, and indeed setdefault makes this the >>>> case. >>> >>> Right. >>> >> Ok. >> >>> The other situation where we can get _INVALID is if libxl__domain_type >>> fails, which it can do. >>> >>> I think this should be handled by having places which call >>> libxl__domain_type abandon operation and return an error if the >>> libxl__domain_type fails. >>> >>> If this is done, then general variables, parameters, etc. within libxl >>> which are supposed to contain a libxl_domain_type will never contain >>> _INVALID. >>> >> I like this. I''ll chase each call to that function and have the calle >> failing if a DOMAIN_TYPE_INVALID is returned. Then, if I go this way, >> can I also nuke both the ''case DOMAIN_TYPE_INVALID'' _and_ the default >> clauses from everywhere? I seem to think I could... > > iff the compiler is smart enough to realise that in the type == INVALID > case you have returned already before reaching the switch statement, > otherwise you will need to have "case INVALID: abort()".What is latest status of this patch? When will it go upstream?> >> >> Thanks and Regards, >> Daio-- ---to satisfy European Law for business letters: Advanced Micro Devices GmbH Einsteinring 24, 85689 Dornach b. Muenchen Geschaeftsfuehrer: Alberto Bozzo, Andrew Bowd Sitz: Dornach, Gemeinde Aschheim, Landkreis Muenchen Registergericht Muenchen, HRB Nr. 43632
Dario Faggioli
2012-Jun-04 14:03 UTC
Re: [PATCH v4] libxl: introduce LIBXL_DOMAIN_TYPE_INVALID
On Mon, 2012-06-04 at 15:11 +0200, Christoph Egger wrote:> What is latest status of this patch? When will it go upstream? >I''m resending a new version of it in a very short while... Let''s see if I manage in intercepting everyone''s taste! :-) Dario -- <<This happens because I choose it to happen!>> (Raistlin Majere) ----------------------------------------------------------------- Dario Faggioli, Ph.D, http://retis.sssup.it/people/faggioli Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK) _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel