| Message ID | d4cf6539ff179e7ade820feadd8088f33da49196.1671111056.git.viresh.kumar@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | libxl: virtio: Fix build error for 32-bit platforms | expand |
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,
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 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 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.
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
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-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 :)
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));
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(-)