| Message ID | 1455908370-12107-1-git-send-email-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>>> On 19.02.16 at 19:59, <andrew.cooper3@citrix.com> wrote: > --- a/tools/firmware/hvmloader/util.h > +++ b/tools/firmware/hvmloader/util.h > @@ -9,6 +9,21 @@ > #include <xen/hvm/hvm_info_table.h> > #include "e820.h" > > +/* Persuade errno.h to give us some un-prefixed values. */ > +#define __XEN_PUBLIC_ERRNO_H__ That's ugly. You would probably better include it the "normal" way once (giving you the XEN_* values), and then a second time ... > +#define XEN_ERRNO(name, value) name = value, > +enum { > +#include <xen/errno.h> > +}; ... this way. Albeit ... > +/* Cause xs_wire.h to give us xsd_errors[]. */ > +#define EINVAL EINVAL > + > +/* Fill in errno values needed by xs_wire.h, missing from errno.h. */ > +#define EISDIR 21 > +#define EROFS 30 > +#define ENOTEMPTY 39 ... hmm, I don't think xs_wire.h means to use XEN errno values, which the reference to these three seems to support. Instead I'm afraid this is a third number space, established for the communication with xenstore, which itself doesn't use Xen's errno.h either. There should be XSD_E* values getting defined in xs_wire.h (or a newly introduced helper header). > --- a/tools/firmware/hvmloader/vnuma.c > +++ b/tools/firmware/hvmloader/vnuma.c > @@ -28,7 +28,6 @@ > #include "util.h" > #include "hypercall.h" > #include "vnuma.h" > -#include <xen/errno.h> > > unsigned int nr_vnodes, nr_vmemranges; > unsigned int *vcpu_to_vnode, *vdistance; > @@ -40,7 +39,7 @@ void init_vnuma_info(void) > struct xen_vnuma_topology_info vnuma_topo = { .domid = DOMID_SELF }; > > rc = hypercall_memory_op(XENMEM_get_vnumainfo, &vnuma_topo); > - if ( rc != -XEN_ENOBUFS ) > + if ( rc != -ENOBUFS ) This change would also be unnecessary if you included xen/errno.h the "ordinary" way first above. Jan
On 22/02/16 11:10, Jan Beulich wrote: >>>> On 19.02.16 at 19:59, <andrew.cooper3@citrix.com> wrote: >> --- a/tools/firmware/hvmloader/util.h >> +++ b/tools/firmware/hvmloader/util.h >> @@ -9,6 +9,21 @@ >> #include <xen/hvm/hvm_info_table.h> >> #include "e820.h" >> >> +/* Persuade errno.h to give us some un-prefixed values. */ >> +#define __XEN_PUBLIC_ERRNO_H__ > That's ugly. You would probably better include it the "normal" way > once (giving you the XEN_* values), and then a second time ... > >> +#define XEN_ERRNO(name, value) name = value, >> +enum { >> +#include <xen/errno.h> >> +}; > ... this way. Albeit ... > >> +/* Cause xs_wire.h to give us xsd_errors[]. */ >> +#define EINVAL EINVAL >> + >> +/* Fill in errno values needed by xs_wire.h, missing from errno.h. */ >> +#define EISDIR 21 >> +#define EROFS 30 >> +#define ENOTEMPTY 39 > ... hmm, I don't think xs_wire.h means to use XEN errno values, > which the reference to these three seems to support. Instead I'm > afraid this is a third number space, established for the communication > with xenstore, which itself doesn't use Xen's errno.h either. There > should be XSD_E* values getting defined in xs_wire.h (or a newly > introduced helper header). The entire point of xsd_errors[] is to turn the errno strings used in the xenstore protocol back into the local representation, whatever that representation is. HVMLoader already has to use XEN errno values for vnuma. It shouldn't be using two different sources of errno... > >> --- a/tools/firmware/hvmloader/vnuma.c >> +++ b/tools/firmware/hvmloader/vnuma.c >> @@ -28,7 +28,6 @@ >> #include "util.h" >> #include "hypercall.h" >> #include "vnuma.h" >> -#include <xen/errno.h> >> >> unsigned int nr_vnodes, nr_vmemranges; >> unsigned int *vcpu_to_vnode, *vdistance; >> @@ -40,7 +39,7 @@ void init_vnuma_info(void) >> struct xen_vnuma_topology_info vnuma_topo = { .domid = DOMID_SELF }; >> >> rc = hypercall_memory_op(XENMEM_get_vnumainfo, &vnuma_topo); >> - if ( rc != -XEN_ENOBUFS ) >> + if ( rc != -ENOBUFS ) > This change would also be unnecessary if you included xen/errno.h > the "ordinary" way first above. ...or indeed, two different representations of the same errno. ~Andrew
>>> On 22.02.16 at 12:24, <andrew.cooper3@citrix.com> wrote: > On 22/02/16 11:10, Jan Beulich wrote: >>>>> On 19.02.16 at 19:59, <andrew.cooper3@citrix.com> wrote: >>> --- a/tools/firmware/hvmloader/util.h >>> +++ b/tools/firmware/hvmloader/util.h >>> @@ -9,6 +9,21 @@ >>> #include <xen/hvm/hvm_info_table.h> >>> #include "e820.h" >>> >>> +/* Persuade errno.h to give us some un-prefixed values. */ >>> +#define __XEN_PUBLIC_ERRNO_H__ >> That's ugly. You would probably better include it the "normal" way >> once (giving you the XEN_* values), and then a second time ... >> >>> +#define XEN_ERRNO(name, value) name = value, >>> +enum { >>> +#include <xen/errno.h> >>> +}; >> ... this way. Albeit ... >> >>> +/* Cause xs_wire.h to give us xsd_errors[]. */ >>> +#define EINVAL EINVAL >>> + >>> +/* Fill in errno values needed by xs_wire.h, missing from errno.h. */ >>> +#define EISDIR 21 >>> +#define EROFS 30 >>> +#define ENOTEMPTY 39 >> ... hmm, I don't think xs_wire.h means to use XEN errno values, >> which the reference to these three seems to support. Instead I'm >> afraid this is a third number space, established for the communication >> with xenstore, which itself doesn't use Xen's errno.h either. There >> should be XSD_E* values getting defined in xs_wire.h (or a newly >> introduced helper header). > > The entire point of xsd_errors[] is to turn the errno strings used in > the xenstore protocol back into the local representation, whatever that > representation is. Oh, right. > HVMLoader already has to use XEN errno values for vnuma. It shouldn't > be using two different sources of errno... > >> >>> --- a/tools/firmware/hvmloader/vnuma.c >>> +++ b/tools/firmware/hvmloader/vnuma.c >>> @@ -28,7 +28,6 @@ >>> #include "util.h" >>> #include "hypercall.h" >>> #include "vnuma.h" >>> -#include <xen/errno.h> >>> >>> unsigned int nr_vnodes, nr_vmemranges; >>> unsigned int *vcpu_to_vnode, *vdistance; >>> @@ -40,7 +39,7 @@ void init_vnuma_info(void) >>> struct xen_vnuma_topology_info vnuma_topo = { .domid = DOMID_SELF }; >>> >>> rc = hypercall_memory_op(XENMEM_get_vnumainfo, &vnuma_topo); >>> - if ( rc != -XEN_ENOBUFS ) >>> + if ( rc != -ENOBUFS ) >> This change would also be unnecessary if you included xen/errno.h >> the "ordinary" way first above. > > ...or indeed, two different representations of the same errno. Okay then. But I'd still prefer to see this #define __XEN_PUBLIC_ERRNO_H__ gone. Even if you don't mean to use XEN_E* values, there's no harm having them declared. Jan
diff --git a/tools/firmware/hvmloader/util.h b/tools/firmware/hvmloader/util.h index 132d915..052de1a 100644 --- a/tools/firmware/hvmloader/util.h +++ b/tools/firmware/hvmloader/util.h @@ -9,6 +9,21 @@ #include <xen/hvm/hvm_info_table.h> #include "e820.h" +/* Persuade errno.h to give us some un-prefixed values. */ +#define __XEN_PUBLIC_ERRNO_H__ +#define XEN_ERRNO(name, value) name = value, +enum { +#include <xen/errno.h> +}; + +/* Cause xs_wire.h to give us xsd_errors[]. */ +#define EINVAL EINVAL + +/* Fill in errno values needed by xs_wire.h, missing from errno.h. */ +#define EISDIR 21 +#define EROFS 30 +#define ENOTEMPTY 39 + #define __STR(...) #__VA_ARGS__ #define STR(...) __STR(__VA_ARGS__) diff --git a/tools/firmware/hvmloader/vnuma.c b/tools/firmware/hvmloader/vnuma.c index 4121cc6..85c1a79 100644 --- a/tools/firmware/hvmloader/vnuma.c +++ b/tools/firmware/hvmloader/vnuma.c @@ -28,7 +28,6 @@ #include "util.h" #include "hypercall.h" #include "vnuma.h" -#include <xen/errno.h> unsigned int nr_vnodes, nr_vmemranges; unsigned int *vcpu_to_vnode, *vdistance; @@ -40,7 +39,7 @@ void init_vnuma_info(void) struct xen_vnuma_topology_info vnuma_topo = { .domid = DOMID_SELF }; rc = hypercall_memory_op(XENMEM_get_vnumainfo, &vnuma_topo); - if ( rc != -XEN_ENOBUFS ) + if ( rc != -ENOBUFS ) return; ASSERT(vnuma_topo.nr_vcpus == hvm_info->nr_vcpus); diff --git a/tools/firmware/hvmloader/xenbus.c b/tools/firmware/hvmloader/xenbus.c index d0ed993..448157d 100644 --- a/tools/firmware/hvmloader/xenbus.c +++ b/tools/firmware/hvmloader/xenbus.c @@ -27,7 +27,6 @@ #include "util.h" #include "hypercall.h" -#include <errno.h> #include <xen/sched.h> #include <xen/event_channel.h> #include <xen/hvm/params.h>
hvmloader is unhosted, and shouldn't use the system errno.h. It already has to use Xen's errno.h for other hypercalls. Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> --- CC: Jan Beulich <JBeulich@suse.com> CC: Ian Campbell <Ian.Campbell@citrix.com> CC: Ian Jackson <Ian.Jackson@eu.citrix.com> CC: Wei Liu <wei.liu2@citrix.com> CC: Doug Goldstein <cardoe@cardoe.com> v2: Fix compilation. I am not sure how v1 compiled, but I did definitely check it before posting. Furthermore, this patch provies that attempting to combine two header files from the Xen public interface is an utter disaster... --- tools/firmware/hvmloader/util.h | 15 +++++++++++++++ tools/firmware/hvmloader/vnuma.c | 3 +-- tools/firmware/hvmloader/xenbus.c | 1 - 3 files changed, 16 insertions(+), 3 deletions(-)