Re: [Xen-devel] [PATCH v2] libxl: ocaml: guard x86-specific functions behind an ifdef

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Sun, 2014-01-12 at 19:17 +0000, Dave Scott wrote:
> Hi Anil,
> 
> Thanks for getting oxenstored on ARM working!
> 
> I'm happy with a simple 'Failure not implemented' exception for the moment. I 
> think that once we're using the libxl bindings everywhere we can probably 
> remove these libxc bindings anyway.
> 
> Acked-by: David Scott <dave.scott@xxxxxxxxxxxxx>

Acked-by: Ian Campbell <ian.campbell@xxxxxxxxxx>

Release hat: This is pretty tightly targeted, and the worst that could
happen is a build failure, which is generally pretty easy to detect
before a release, although perhaps less so in the case of ocaml which
isn't enabled by everyone.

Although I have two misgivings in that regard:

> > diff --git a/tools/ocaml/libs/xc/xenctrl_stubs.c
> > b/tools/ocaml/libs/xc/xenctrl_stubs.c
> > index f5cf0ed..ff29b47 100644
> > --- a/tools/ocaml/libs/xc/xenctrl_stubs.c
> > +++ b/tools/ocaml/libs/xc/xenctrl_stubs.c
> > @@ -714,6 +714,7 @@ CAMLprim value stub_xc_domain_cpuid_set(value
> > xch, value domid,  {
> >     CAMLparam4(xch, domid, input, config);
> >     CAMLlocal2(array, tmp);

Aren't these variables now unused? Does the compiler not complain about
this (with -Werror => build failure)?

Perhaps CAMLlocal2 both defines and references the variables keeping
this issue at bay?

> > +#if defined(__i386__) || defined(__x86_64__)
> >     int r;
> >     unsigned int c_input[2];
> >     char *c_config[4], *out_config[4];
> > @@ -742,17 +743,24 @@ CAMLprim value stub_xc_domain_cpuid_set(value
> > xch, value domid,
> >                      c_input, (const char **)c_config, out_config);
> >     if (r < 0)
> >             failwith_xc(_H(xch));
> > +#else
> > +   caml_failwith("xc_domain_cpuid_set: not implemented");
> > #endif
> >     CAMLreturn(array);

In the !__i386__ && !__x86_64__ case this code is unreachable, right
because caml_failwith is marked Noreturn?

Is there any chance that some compiler version might pick up on this and
complain about the dead code? Or perhaps complain about the
uninitialised use of array?

I suppose putting the CAMLreturn inside the x86 case runs the opposite
risk of a compiler which doesn't pay proper attention to Noreturn and
therefore things we are reaching the end of a non-void function? Would
CAMLreturn(unit) be appropriate in that case?

> >  }
> > 
> >  CAMLprim value stub_xc_domain_cpuid_apply_policy(value xch, value
> > domid)  {
> >     CAMLparam2(xch, domid);
> > +#if defined(__i386__) || defined(__x86_64__)
> >     int r;
> > 
> >     r = xc_cpuid_apply_policy(_H(xch), _D(domid));
> >     if (r < 0)
> >             failwith_xc(_H(xch));
> > +#else
> > +   caml_failwith("xc_domain_cpuid_apply_policy: not implemented");
> > #endif
> >     CAMLreturn(Val_unit);

Unreached?

> >  }
> > 
> > @@ -760,6 +768,7 @@ CAMLprim value stub_xc_cpuid_check(value xch,
> > value input, value config)  {
> >     CAMLparam3(xch, input, config);
> >     CAMLlocal3(ret, array, tmp);

Unused?

> > +#if defined(__i386__) || defined(__x86_64__)
> >     int r;
> >     unsigned int c_input[2];
> >     char *c_config[4], *out_config[4];
> > @@ -792,6 +801,9 @@ CAMLprim value stub_xc_cpuid_check(value xch,
> > value input, value config)
> >     Store_field(ret, 0, Val_bool(r));
> >     Store_field(ret, 1, array);
> > 
> > +#else
> > +   caml_failwith("xc_domain_cpuid_check: not implemented");
> > #endif
> >     CAMLreturn(ret);

Unreached?

> >  }
> > 
> > --
> > 1.8.1.2
> > 
> > 
> > --
> > Anil Madhavapeddy                                 http://anil.recoil.org
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@xxxxxxxxxxxxx
> http://lists.xen.org/xen-devel



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel