The field 'base' in 'struct libxl_device_virtio' is defined as uint64,
while we are printing it with '%lu', which is 32bit only 32-bit
platforms. And so generates a error like:

  libxl_internal.h:4388:51: error: format '%lu' expects argument of type 'long
  unsigned int', but argument 3 has type 'uint64_t' {aka 'long long unsigned
  int'} [-Werror=format=]

Fix the same by using PRIx64 instead.

Now that the base name is available in hexadecimal format, prefix it
with '0x' as well, which strtoul() also depends upon since base passed
is 0.

Fixes: 43ba5202e2ee ("libxl: add support for generic virtio device")
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
Couldn't test on 32-bit platforms yet, but works fine for 64 bit one.

 tools/libs/light/libxl_virtio.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/libs/light/libxl_virtio.c b/tools/libs/light/libxl_virtio.c
index 6a38def2faf5..2217bda8a253 100644
--- a/tools/libs/light/libxl_virtio.c
+++ b/tools/libs/light/libxl_virtio.c
@@ -45,12 +45,12 @@ static int libxl__set_xenstore_virtio(libxl__gc *gc, uint32_t domid,
     const char *transport = libxl_virtio_transport_to_string(virtio->transport);
 
     flexarray_append_pair(back, "irq", GCSPRINTF("%u", virtio->irq));
-    flexarray_append_pair(back, "base", GCSPRINTF("%lu", virtio->base));
+    flexarray_append_pair(back, "base", GCSPRINTF("0x%"PRIx64, virtio->base));
     flexarray_append_pair(back, "type", GCSPRINTF("%s", virtio->type));
     flexarray_append_pair(back, "transport", GCSPRINTF("%s", transport));
 
     flexarray_append_pair(front, "irq", GCSPRINTF("%u", virtio->irq));
-    flexarray_append_pair(front, "base", GCSPRINTF("%lu", virtio->base));
+    flexarray_append_pair(front, "base", GCSPRINTF("0x%"PRIx64, virtio->base));
     flexarray_append_pair(front, "type", GCSPRINTF("%s", virtio->type));
     flexarray_append_pair(front, "transport", GCSPRINTF("%s", transport));
 
-- 
2.31.1.272.g89b43f80a514

On 15/12/2022 1:31 pm, Viresh Kumar wrote:
> The field 'base' in 'struct libxl_device_virtio' is defined as uint64,
> while we are printing it with '%lu', which is 32bit only 32-bit
> platforms. And so generates a error like:
>
>   libxl_internal.h:4388:51: error: format '%lu' expects argument of type 'long
>   unsigned int', but argument 3 has type 'uint64_t' {aka 'long long unsigned
>   int'} [-Werror=format=]
>
> Fix the same by using PRIx64 instead.
>
> Now that the base name is available in hexadecimal format, prefix it
> with '0x' as well, which strtoul() also depends upon since base passed
> is 0.
>
> Fixes: 43ba5202e2ee ("libxl: add support for generic virtio device")
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>

In order to unblock OSSTest, I've committed this with an adjusted commit
message, with the agreement on Anthony on IRC.

~Andrew

On 15.12.2022 14:31, Viresh Kumar wrote:
> The field 'base' in 'struct libxl_device_virtio' is defined as uint64,
> while we are printing it with '%lu', which is 32bit only 32-bit
> platforms. And so generates a error like:
> 
>   libxl_internal.h:4388:51: error: format '%lu' expects argument of type 'long
>   unsigned int', but argument 3 has type 'uint64_t' {aka 'long long unsigned
>   int'} [-Werror=format=]
> 
> Fix the same by using PRIx64 instead.
> 
> Now that the base name is available in hexadecimal format, prefix it
> with '0x' as well,

Which might better be done using "%#"PRIx64 ...

Jan

On Thu, Dec 15, 2022 at 07:01:40PM +0530, Viresh Kumar wrote:
> The field 'base' in 'struct libxl_device_virtio' is defined as uint64,
> while we are printing it with '%lu', which is 32bit only 32-bit
> platforms. And so generates a error like:
> 
>   libxl_internal.h:4388:51: error: format '%lu' expects argument of type 'long
>   unsigned int', but argument 3 has type 'uint64_t' {aka 'long long unsigned
>   int'} [-Werror=format=]
> 
> Fix the same by using PRIx64 instead.
> 
> Now that the base name is available in hexadecimal format, prefix it
> with '0x' as well, which strtoul() also depends upon since base passed
> is 0.
> 
> Fixes: 43ba5202e2ee ("libxl: add support for generic virtio device")
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
> Couldn't test on 32-bit platforms yet, but works fine for 64 bit one.
> 
>  tools/libs/light/libxl_virtio.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/libs/light/libxl_virtio.c b/tools/libs/light/libxl_virtio.c
> index 6a38def2faf5..2217bda8a253 100644
> --- a/tools/libs/light/libxl_virtio.c
> +++ b/tools/libs/light/libxl_virtio.c
> @@ -45,12 +45,12 @@ static int libxl__set_xenstore_virtio(libxl__gc *gc, uint32_t domid,
>      const char *transport = libxl_virtio_transport_to_string(virtio->transport);
>  
>      flexarray_append_pair(back, "irq", GCSPRINTF("%u", virtio->irq));
> -    flexarray_append_pair(back, "base", GCSPRINTF("%lu", virtio->base));
> +    flexarray_append_pair(back, "base", GCSPRINTF("0x%"PRIx64, virtio->base));

There is also PRIu64 that exist, which would be perfect to replace %u.
Could we use that instead?

I'd rather not have to think about which base is used for numbers in
xenstore. I can't find any hexadecimal numbers in xenstore for a simple
guest at the moment, so probably best to avoid adding one. And using
hexadecimal isn't needed to fix the build.

Thanks,

-- 
Anthony PERARD

On 15.12.2022 14:48, Anthony PERARD wrote:
> On Thu, Dec 15, 2022 at 07:01:40PM +0530, Viresh Kumar wrote:
>> The field 'base' in 'struct libxl_device_virtio' is defined as uint64,
>> while we are printing it with '%lu', which is 32bit only 32-bit
>> platforms. And so generates a error like:
>>
>>   libxl_internal.h:4388:51: error: format '%lu' expects argument of type 'long
>>   unsigned int', but argument 3 has type 'uint64_t' {aka 'long long unsigned
>>   int'} [-Werror=format=]
>>
>> Fix the same by using PRIx64 instead.
>>
>> Now that the base name is available in hexadecimal format, prefix it
>> with '0x' as well, which strtoul() also depends upon since base passed
>> is 0.
>>
>> Fixes: 43ba5202e2ee ("libxl: add support for generic virtio device")
>> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
>> ---
>> Couldn't test on 32-bit platforms yet, but works fine for 64 bit one.
>>
>>  tools/libs/light/libxl_virtio.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/tools/libs/light/libxl_virtio.c b/tools/libs/light/libxl_virtio.c
>> index 6a38def2faf5..2217bda8a253 100644
>> --- a/tools/libs/light/libxl_virtio.c
>> +++ b/tools/libs/light/libxl_virtio.c
>> @@ -45,12 +45,12 @@ static int libxl__set_xenstore_virtio(libxl__gc *gc, uint32_t domid,
>>      const char *transport = libxl_virtio_transport_to_string(virtio->transport);
>>  
>>      flexarray_append_pair(back, "irq", GCSPRINTF("%u", virtio->irq));
>> -    flexarray_append_pair(back, "base", GCSPRINTF("%lu", virtio->base));
>> +    flexarray_append_pair(back, "base", GCSPRINTF("0x%"PRIx64, virtio->base));
> 
> There is also PRIu64 that exist, which would be perfect to replace %u.
> Could we use that instead?
> 
> I'd rather not have to think about which base is used for numbers in
> xenstore. I can't find any hexadecimal numbers in xenstore for a simple
> guest at the moment, so probably best to avoid adding one. And using
> hexadecimal isn't needed to fix the build.

Otoh an address formatted in decimal is pretty unusable to any human
(who might be inspecting xenstore for whatever reasons).

Jan

On 15/12/2022 3:11 pm, Jan Beulich wrote:
> On 15.12.2022 14:48, Anthony PERARD wrote:
>> On Thu, Dec 15, 2022 at 07:01:40PM +0530, Viresh Kumar wrote:
>>> The field 'base' in 'struct libxl_device_virtio' is defined as uint64,
>>> while we are printing it with '%lu', which is 32bit only 32-bit
>>> platforms. And so generates a error like:
>>>
>>>   libxl_internal.h:4388:51: error: format '%lu' expects argument of type 'long
>>>   unsigned int', but argument 3 has type 'uint64_t' {aka 'long long unsigned
>>>   int'} [-Werror=format=]
>>>
>>> Fix the same by using PRIx64 instead.
>>>
>>> Now that the base name is available in hexadecimal format, prefix it
>>> with '0x' as well, which strtoul() also depends upon since base passed
>>> is 0.
>>>
>>> Fixes: 43ba5202e2ee ("libxl: add support for generic virtio device")
>>> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
>>> ---
>>> Couldn't test on 32-bit platforms yet, but works fine for 64 bit one.
>>>
>>>  tools/libs/light/libxl_virtio.c | 4 ++--
>>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/tools/libs/light/libxl_virtio.c b/tools/libs/light/libxl_virtio.c
>>> index 6a38def2faf5..2217bda8a253 100644
>>> --- a/tools/libs/light/libxl_virtio.c
>>> +++ b/tools/libs/light/libxl_virtio.c
>>> @@ -45,12 +45,12 @@ static int libxl__set_xenstore_virtio(libxl__gc *gc, uint32_t domid,
>>>      const char *transport = libxl_virtio_transport_to_string(virtio->transport);
>>>  
>>>      flexarray_append_pair(back, "irq", GCSPRINTF("%u", virtio->irq));
>>> -    flexarray_append_pair(back, "base", GCSPRINTF("%lu", virtio->base));
>>> +    flexarray_append_pair(back, "base", GCSPRINTF("0x%"PRIx64, virtio->base));
>> There is also PRIu64 that exist, which would be perfect to replace %u.
>> Could we use that instead?
>>
>> I'd rather not have to think about which base is used for numbers in
>> xenstore. I can't find any hexadecimal numbers in xenstore for a simple
>> guest at the moment, so probably best to avoid adding one. And using
>> hexadecimal isn't needed to fix the build.
> Otoh an address formatted in decimal is pretty unusable to any human
> (who might be inspecting xenstore for whatever reasons).

A consumer of xenstore has to cope with all bases anyway.  Anything that
doesn't is broken.

Keys ought to be expressed in the logical base for a human to read, and
hex for base is the right one in this case.

~Andrew

On Thu, Dec 15, 2022 at 04:58:02PM +0000, Andrew Cooper wrote:
> A consumer of xenstore has to cope with all bases anyway.  Anything that
> doesn't is broken.

So libxl is broken, that good to know :-). Most keys read by libxl are
expected to be base 10, with some allowed to be in different base (as
they're a few that uses strtoul(,,0);)

So don't try to change the base of existing keys ;-).

For those virtio one in particular, it's probably ok. libxl doesn't
mind, and hopefully the consumer of those don't mind either.

-- 
Anthony PERARD

On 15-12-22, 17:33, Anthony PERARD wrote:
> For those virtio one in particular, it's probably ok. libxl doesn't
> mind, and hopefully the consumer of those don't mind either.

FWIW, the consumer in this case, Rust based xen-vhost-frontent [1]
implementation, did break and I still need to fix it to read hex
properly :)

-- 
viresh

[1] https://github.com/vireshk/xen-vhost-frontend

On 15/12/2022 5:33 pm, Anthony PERARD wrote:
> On Thu, Dec 15, 2022 at 04:58:02PM +0000, Andrew Cooper wrote:
>> A consumer of xenstore has to cope with all bases anyway.  Anything that
>> doesn't is broken.
> So libxl is broken, that good to know :-).

Yes.  Really, yes.

This is sufficiently basic stuff for text based APIs/ABIs that it ought
to go without saying.

>  Most keys read by libxl are
> expected to be base 10, with some allowed to be in different base (as
> they're a few that uses strtoul(,,0);)

This is at least recoverable by switching to ,,0 uniformly.

That said, xenstore-paths.pandoc's attempt to describe the grammar
appears to be ambiguous.

That's the first place to fix.  I'll put a ticket on gitlab because I
don't have enough cycles to do this now.

~Andrew