[PATCH v2] hvmloader: Use xen/errno.h rather than the host systems errno.h

[PATCH v2] hvmloader: Use xen/errno.h rather than the host systems errno.h

[Andrew Cooper]

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(-)

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>
-- 
2.1.4

Re: [PATCH v2] hvmloader: Use xen/errno.h rather than the host systems errno.h

[Jan Beulich]

>>> 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

Re: [PATCH v2] hvmloader: Use xen/errno.h rather than the host systems errno.h

[Andrew Cooper]

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

Re: [PATCH v2] hvmloader: Use xen/errno.h rather than the host systems errno.h

[Jan Beulich]

>>> 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