Messages in this thread | ![/](/images/icornerl.gif) | | Date | Mon, 6 Jul 2015 12:05:06 +0200 | From | Thomas Huth <> | Subject | Re: [PATCH RESEND] virtio: Fix typecast of pointer in vring_init() |
| |
On Mon, 6 Jul 2015 12:50:22 +0300 "Michael S. Tsirkin" <mst@redhat.com> wrote:
> On Mon, Jul 06, 2015 at 11:24:42AM +0200, Thomas Huth wrote: > > On Sun, 5 Jul 2015 14:59:54 +0200 > > "Michael S. Tsirkin" <mst@redhat.com> wrote: > > > > > On Sun, Jul 05, 2015 at 12:58:53PM +0200, Michael S. Tsirkin wrote: > > > > On Thu, Jul 02, 2015 at 09:21:22AM +0200, Thomas Huth wrote: > > > > > The virtio_ring.h header is used in userspace programs (ie. QEMU), > > > > > too. Here we can not assume that sizeof(pointer) is the same as > > > > > sizeof(long), e.g. when compiling for Windows, so the typecast in > > > > > vring_init() should be done with (uintptr_t) instead of (unsigned long). > > > > > > > > > > Signed-off-by: Thomas Huth <thuth@redhat.com> > > > > > > > > This seems to break some userspace too: > > > > > > > > INSTALL usr/include/linux/ (413 files) > > > > CHECK usr/include/linux/ (413 files) > > > > HOSTCC Documentation/accounting/getdelays > > > > HOSTCC Documentation/connector/ucon > > > > HOSTCC Documentation/mic/mpssd/mpssd.o > > > > In file included from Documentation/mic/mpssd/mpssd.c:36:0: > > > > ./usr/include/linux/virtio_ring.h: In function ‘vring_init’: > > > > ./usr/include/linux/virtio_ring.h:146:24: error: ‘uintptr_t’ undeclared > > > > (first use in this function) > > > > vr->used = (void *)(((uintptr_t)&vr->avail->ring[num] + > > > > sizeof(__virtio16) > > > > ^ > > > > ./usr/include/linux/virtio_ring.h:146:24: note: each undeclared > > > > identifier is reported only once for each function it appears in > > > > scripts/Makefile.host:108: recipe for target > > > > D'oh, I only built the kernel for powerpc, that's why I did not notice > > this breakage in the MIC code. Sorry! > > > > > > E.g. fuse.h has this code: > > > > #ifdef __KERNEL__ > > > > #include <linux/types.h> > > > > #else > > > > #include <stdint.h> > > > > #endif > > > > > > > > Maybe we need something similar. > > > > > > The following seems to help. Does it help the windows build? > > ... > > > diff --git a/include/uapi/linux/virtio_ring.h b/include/uapi/linux/virtio_ring.h > > > index 8682551..c072959 100644 > > > --- a/include/uapi/linux/virtio_ring.h > > > +++ b/include/uapi/linux/virtio_ring.h > > > @@ -31,6 +31,9 @@ > > > * SUCH DAMAGE. > > > * > > > * Copyright Rusty Russell IBM Corporation 2007. */ > > > +#ifndef __KERNEL__ > > > +#include <stdint.h> > > > +#endif > > > #include <linux/types.h> > > > #include <linux/virtio_types.h> > > > > Since the update-linux-headers.sh script from QEMU replaces the > > "#include <linux/types.h>" in virtio_ring.h to include a file that again > > includes stdint.h already, your above patch should not do any > > difference for compiling QEMU for Windows, I think. It's really just > > about replacing the typecast there. > > > > But you're right of course for other user space applications which > > include this header by other means. So I'm fine if you change my patch > > with your hunk above (or add it as an additional patch). Or shall I > > rather resend my patch with your above hunk included? > > > > Thomas > > You don't have to, but could you confirm that stdint has uintptr_t on > mingw? >
$ grep typedef.*uintptr /usr/x86_64-w64-mingw32/sys-root/mingw/include/*.h ... /usr/x86_64-w64-mingw32/sys-root/mingw/include/crtdefs.h:__MINGW_EXTENSION typedef unsigned __int64 uintptr_t; ...
And the stdint.h from my MinGW installation includes crtdefs.h. So yes, confirmed that stdint.h provides uintptr_t there, too.
Thomas
| ![\](/images/icornerr.gif) |