| Message ID | 1395404743-13833-1-git-send-email-julien.grall@linaro.org |
|---|---|
| State | New |
| Headers | show |
On Fri, 2014-03-21 at 12:25 +0000, Julien Grall wrote: > 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 "checking" > be always false. Therefore both GCC and clang will never go to the error > case. Are there any API compatibility concerns here? I think there isn't because enum foo bar = get_foo() and int bar = get_foo() are both always valid whether get_foo() returns an enum foo or an int. So I think we can make this change without a LIBXL_HAVE define. Would be good to confirm and include the rationale in the commit log. Ian.
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.
Thanks,
Ian.
Ian Campbell writes ("Re: [PATCH v2] tools/libxl: libxl_get_scheduler should return an int"):
> Are there any API compatibility concerns here? I think there isn't
> because
> enum foo bar = get_foo()
> and
> int bar = get_foo()
> are both always valid whether get_foo() returns an enum foo or an int.
Indeed. In theory there might be code which relies on this enum being
an unsigned type but it is much more likely that any other code where
the meaning changes was wrong before and will be right afterwards.
> So I think we can make this change without a LIBXL_HAVE define. Would be
> good to confirm and include the rationale in the commit log.
Yes.
Ian.
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,
On 03/21/2014 12:43 PM, Ian Campbell wrote: > On Fri, 2014-03-21 at 12:25 +0000, Julien Grall wrote: >> 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 > > "checking" I will update the commit message. >> be always false. Therefore both GCC and clang will never go to the error >> case. > > Are there any API compatibility concerns here? I think there isn't > because > enum foo bar = get_foo() > and > int bar = get_foo() > are both always valid whether get_foo() returns an enum foo or an int. > > So I think we can make this change without a LIBXL_HAVE define. Would be > good to confirm and include the rationale in the commit log. C99, 6.7.2.2p4 Each enumerated type shall be compatible with char, a signed integer type, or an unsigned integer type. The choice of type is implementation-defined,108) but shall be capable of representing the values of all the members of the enumeration. [...] I gave a try with a simple test file: typedef enum libxl_sched { SCHED_SCHED, } libxl_sched; int f(void); int main(void) { libxl_sched sched = f(); return 0; } I compiled twice, the second time I have changed f return type. The result is the same with gcc 4.7 on x86. For ARM with gcc 4.6.3 there is an extra move on the first version. IHMO it's not harmfull. Regards
Julien Grall writes ("Re: [PATCH v2] tools/libxl: libxl_get_scheduler should return an int"):
> On 03/21/2014 02:28 PM, Ian Jackson wrote:
> > 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).
Ah. I think libxl_scheduler_from_string should be changed too.
Having enums in APIs is not a good idea. One reason is that on some
architectures the size or passing convention can depend on the range
of the enum. So backporting a patch that adds a new _value_ to an
enum can result in ABI breakage, even if the new value is never
actually used!
I realise that this is rather counterintuitive and I'm sorry I didn't
spot this when it went in.
Ian.
Julien Grall writes ("Re: [PATCH v2] tools/libxl: libxl_get_scheduler should return an int"):
> On 03/21/2014 12:43 PM, Ian Campbell wrote:
> > Are there any API compatibility concerns here? I think there isn't
> > because
> > enum foo bar = get_foo()
> > and
> > int bar = get_foo()
> > are both always valid whether get_foo() returns an enum foo or an int.
> >
> > So I think we can make this change without a LIBXL_HAVE define. Would be
> > good to confirm and include the rationale in the commit log.
>
> C99, 6.7.2.2p4
>
> Each enumerated type shall be compatible with char, a signed integer
> type, or an unsigned integer type. The choice of type is
> implementation-defined,108) but shall be capable of representing the
> values of all the members of the enumeration. [...]
This is quite a weak statement, and I don't think it's helpful as part
of an argument that this change has no API implications.
As you have discovered, some enums can be unsigned types. Changing
the return type of this function to from an unsigned to signed type
can change the meaning of the code which uses it.
My justification for making this change is that where the meaning
changes, it is very likely to do so in a way which better serve's the
programmer's intent.
> I gave a try with a simple test file:
> typedef enum libxl_sched
> {
> SCHED_SCHED,
> } libxl_sched;
>
> int f(void);
>
> int main(void)
> {
> libxl_sched sched = f();
>
> return 0;
> }
>
> I compiled twice, the second time I have changed f return type.
>
> The result is the same with gcc 4.7 on x86. For ARM with gcc 4.6.3 there
> is an extra move on the first version. IHMO it's not harmfull.
Here you are discussing ABI, rather than API, compatibility.
A compiler which sees an enum type is allowed to assume that it's
within the enum's range. That might generate different code in subtle
situations. That you don't see any difference now is not conclusive.
However, again, I would argue that the change is purely beneficial.
Forbidding the compiler from making these assumptions about the values
of objects with enum type will make the code more correct, not less.
Thanks,
Ian.
On 03/21/2014 03:02 PM, Ian Jackson wrote: > Julien Grall writes ("Re: [PATCH v2] tools/libxl: libxl_get_scheduler should return an int"): >> On 03/21/2014 02:28 PM, Ian Jackson wrote: >>> 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). > > Ah. I think libxl_scheduler_from_string should be changed too. libxl_scheduler_from_string is similar to all libxl*_from_string enum function. If we change this function we should also modify libxl_bios_type_from_string, libxl_timer_mode_from_string... Regards,
Julien Grall writes ("Re: [PATCH v2] tools/libxl: libxl_get_scheduler should return an int"):
> On 03/21/2014 03:02 PM, Ian Jackson wrote:
> > Ah. I think libxl_scheduler_from_string should be changed too.
>
> libxl_scheduler_from_string is similar to all libxl*_from_string enum
> function. If we change this function we should also modify
> libxl_bios_type_from_string, libxl_timer_mode_from_string...
Yes...
Ian.
On 03/21/2014 03:21 PM, Ian Jackson wrote: > Julien Grall writes ("Re: [PATCH v2] tools/libxl: libxl_get_scheduler should return an int"): >> On 03/21/2014 03:02 PM, Ian Jackson wrote: >>> Ah. I think libxl_scheduler_from_string should be changed too. >> >> libxl_scheduler_from_string is similar to all libxl*_from_string enum >> function. If we change this function we should also modify >> libxl_bios_type_from_string, libxl_timer_mode_from_string... > > Yes... I'm not sure to understand, do you I need to do a s/enum .../int/? If so, there will be some issue on every caller to this function. Because enum * is not compatible with int *.
On Fri, 2014-03-21 at 14:28 +0000, 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 did you intend to say anything other than "have some comments on the xl part" here? > > > 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. > > Thanks, > Ian.
diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c index 30b0b06..f1adc42 100644 --- a/tools/libxl/libxl.c +++ b/tools/libxl/libxl.c @@ -4725,7 +4725,7 @@ int libxl_set_vcpuonline(libxl_ctx *ctx, uint32_t domid, libxl_bitmap *cpumap) return rc; } -libxl_scheduler libxl_get_scheduler(libxl_ctx *ctx) +int libxl_get_scheduler(libxl_ctx *ctx) { libxl_scheduler sched, ret; diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h index b2c3015..a408950 100644 --- a/tools/libxl/libxl.h +++ b/tools/libxl/libxl.h @@ -1079,7 +1079,7 @@ int libxl_domain_get_nodeaffinity(libxl_ctx *ctx, uint32_t domid, libxl_bitmap *nodemap); int libxl_set_vcpuonline(libxl_ctx *ctx, uint32_t domid, libxl_bitmap *cpumap); -libxl_scheduler libxl_get_scheduler(libxl_ctx *ctx); +int libxl_get_scheduler(libxl_ctx *ctx); /* Per-scheduler parameters */ int libxl_sched_credit_params_get(libxl_ctx *ctx, uint32_t poolid, 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; 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; } if (libxl_get_freecpus(ctx, &freemap)) {
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. 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> --- Ian, it seems this is the only place with this such construction. Changes in v2: - Was "Correctly check if libxl_get_scheduler has failed" --- tools/libxl/libxl.c | 2 +- tools/libxl/libxl.h | 2 +- tools/libxl/xl_cmdimpl.c | 6 ++++-- 3 files changed, 6 insertions(+), 4 deletions(-)