| Message ID | 20161228145344.30819-1-cov@codeaurora.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 12/28/2016 08:53 AM, Christopher Covington wrote: > The definition of the major() and minor() macros are moving within glibc to > <sys/sysmacros.h>. Include this header to avoid the following sorts of > build-stopping messages: > > The additional include allows the build to complete on Fedora 26 (Rawhide) > with glibc version 2.24.90. > > Signed-off-by: Christopher Covington <cov@codeaurora.org> > --- > include/sysemu/os-posix.h | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/include/sysemu/os-posix.h b/include/sysemu/os-posix.h > index b0a6c0695b..772d58f7ed 100644 > --- a/include/sysemu/os-posix.h > +++ b/include/sysemu/os-posix.h > @@ -28,6 +28,7 @@ > > #include <sys/mman.h> > #include <sys/socket.h> > +#include <sys/sysmacros.h> I repeat what I said on v1: Works for glibc; but <sys/sysmacros.h> is non-standard and not present on some other systems, so this may fail to build elsewhere. You'll probably need a configure probe. Autoconf also says that some platforms have <sys/mkdev.h> instead of <sys/sysmacros.h> (per its AC_HEADER_MAJOR macro).
On 28 December 2016 at 16:10, Eric Blake <eblake@redhat.com> wrote: > On 12/28/2016 08:53 AM, Christopher Covington wrote: >> The definition of the major() and minor() macros are moving within glibc to >> <sys/sysmacros.h>. Include this header to avoid the following sorts of >> build-stopping messages: >> > >> The additional include allows the build to complete on Fedora 26 (Rawhide) >> with glibc version 2.24.90. >> >> Signed-off-by: Christopher Covington <cov@codeaurora.org> >> --- >> include/sysemu/os-posix.h | 1 + >> 1 file changed, 1 insertion(+) >> >> diff --git a/include/sysemu/os-posix.h b/include/sysemu/os-posix.h >> index b0a6c0695b..772d58f7ed 100644 >> --- a/include/sysemu/os-posix.h >> +++ b/include/sysemu/os-posix.h >> @@ -28,6 +28,7 @@ >> >> #include <sys/mman.h> >> #include <sys/socket.h> >> +#include <sys/sysmacros.h> > > I repeat what I said on v1: > > Works for glibc; but <sys/sysmacros.h> is non-standard and not present > on some other systems, so this may fail to build elsewhere. You'll > probably need a configure probe. Autoconf also says that some platforms > have <sys/mkdev.h> instead of <sys/sysmacros.h> (per its AC_HEADER_MAJOR > macro). Also this seems straightforwardly like a bug in glibc: it shouldn't be making this kind of breaking change. makedev(3) on my Linux box says nothing about needing sysmacros.h for these. thanks -- PMM
Hi Eric, On 12/28/2016 11:10 AM, Eric Blake wrote: > On 12/28/2016 08:53 AM, Christopher Covington wrote: >> The definition of the major() and minor() macros are moving within glibc to >> <sys/sysmacros.h>. Include this header to avoid the following sorts of >> build-stopping messages: >> > >> The additional include allows the build to complete on Fedora 26 (Rawhide) >> with glibc version 2.24.90. >> >> Signed-off-by: Christopher Covington <cov@codeaurora.org> >> --- >> include/sysemu/os-posix.h | 1 + >> 1 file changed, 1 insertion(+) >> >> diff --git a/include/sysemu/os-posix.h b/include/sysemu/os-posix.h >> index b0a6c0695b..772d58f7ed 100644 >> --- a/include/sysemu/os-posix.h >> +++ b/include/sysemu/os-posix.h >> @@ -28,6 +28,7 @@ >> >> #include <sys/mman.h> >> #include <sys/socket.h> >> +#include <sys/sysmacros.h> > > I repeat what I said on v1: > > Works for glibc; but <sys/sysmacros.h> is non-standard and not present > on some other systems, so this may fail to build elsewhere. I read your response to v1 but got stuck on this "some other systems" statement which seems too vague for me to act on. I see the following operating systems checked in configure: Cygwin, mingw32, GNU/kFreeBSD, FreeBSD, DragonFly, NetBSD, OpenBSD, Darwin, SunOS, AIX, Haiku, and Linux. But I'm really not sure what list of C libraries and corresponding mkdev.h versus sysmacros.h versus types.h usage this translates to. > You'll probably need a configure probe. I'm testing that now and will hopefully send it out as v3 shortly. > Autoconf also says that some platforms have <sys/mkdev.h> instead of > <sys/sysmacros.h> (per its AC_HEADER_MAJOR macro). `git grep mkdev` returns no results for me so I conclude that no currently supported OS/libc requires it. In case anyone wants to work around these messages, I'd like to highlight the --disable-werror option to ./configure. If I had known about it this morning, I probably would be happily authoring other changes right now. Thanks, Cov
On 12/28/2016 11:07 AM, Peter Maydell wrote: > On 28 December 2016 at 16:10, Eric Blake <eblake@redhat.com> wrote: >> On 12/28/2016 08:53 AM, Christopher Covington wrote: >>> The definition of the major() and minor() macros are moving within glibc to >>> <sys/sysmacros.h>. Include this header to avoid the following sorts of >>> build-stopping messages: >> Works for glibc; but <sys/sysmacros.h> is non-standard and not present >> on some other systems, so this may fail to build elsewhere. You'll >> probably need a configure probe. Autoconf also says that some platforms >> have <sys/mkdev.h> instead of <sys/sysmacros.h> (per its AC_HEADER_MAJOR >> macro). > > Also this seems straightforwardly like a bug in glibc: it shouldn't > be making this kind of breaking change. makedev(3) on my Linux box > says nothing about needing sysmacros.h for these. Here's the bug explaining the rationale behind the change: https://sourceware.org/bugzilla/show_bug.cgi?id=19239 It IS a bug fix, but in the other direction - it is fixing namespace pollution that was present in <sys/types.h> and breaking certain standard-required idioms. There HAS been warning, but system man pages have not always been updated to track upstream development, and the plan was that glibc 2.25 only causes a warning rather than outright failure to compile (although with -Werror, you have to adjust right away, rather than when the future glibc actually changes <sys/types.h> again to completely drop the pollution). It is CORRECT to fix any software relying on makedev() to use the CORRECT headers for that declaration, rather than getting it for free from <sys/types.h> pollution - the problem is that makedev() is not portable, and therefore the spelling of the correct header is not trivial - it requires some configure-time probing.
On 29 December 2016 at 13:48, Eric Blake <eblake@redhat.com> wrote: > On 12/28/2016 11:07 AM, Peter Maydell wrote: >> Also this seems straightforwardly like a bug in glibc: it shouldn't >> be making this kind of breaking change. makedev(3) on my Linux box >> says nothing about needing sysmacros.h for these. > > Here's the bug explaining the rationale behind the change: > > https://sourceware.org/bugzilla/show_bug.cgi?id=19239 > > It IS a bug fix, but in the other direction - it is fixing namespace > pollution that was present in <sys/types.h> and breaking certain > standard-required idioms. There HAS been warning, but system man pages > have not always been updated to track upstream development, and the plan > was that glibc 2.25 only causes a warning rather than outright failure > to compile (although with -Werror, you have to adjust right away, rather > than when the future glibc actually changes <sys/types.h> again to > completely drop the pollution). > > It is CORRECT to fix any software relying on makedev() to use the > CORRECT headers for that declaration, rather than getting it for free > from <sys/types.h> pollution - the problem is that makedev() is not > portable, and therefore the spelling of the correct header is not > trivial - it requires some configure-time probing. I checked a FreeBSD manpage and they don't put these macros in sysmacros.h, they're in sys/types.h. I think the "correct" header for them is sys/types.h, because that's where they've always lived and where they've been documented to be. Otherwise you're breaking API compatibility. http://minnie.tuhs.org/cgi-bin/utree.pl?file=2.9BSD/usr/include/sys/types.h shows that sys/types.h is where these macros have lived way back to 2.9BSD in 1983. If POSIX requires sys/types.h not to provide these macros then I think you could argue that that's a bug in the POSIX spec, because it wasn't supposed to make major existing implementations like the BSD family non-compliant. For glibc to move to putting these macros somewhere weird and different to everything else that implements them is just forcing all programs that use them to add extra configure machinery and ifdefs for no good reason. QEMU now has to work around this glibc infelicity, of course, but it would be better if it didn't have to. Breaking existing code that was following the docs is a big deal, and glibc should basically never do it in my view. thanks -- PMM
diff --git a/include/sysemu/os-posix.h b/include/sysemu/os-posix.h index b0a6c0695b..772d58f7ed 100644 --- a/include/sysemu/os-posix.h +++ b/include/sysemu/os-posix.h @@ -28,6 +28,7 @@ #include <sys/mman.h> #include <sys/socket.h> +#include <sys/sysmacros.h> #include <netinet/in.h> #include <netinet/tcp.h> #include <arpa/inet.h>
The definition of the major() and minor() macros are moving within glibc to <sys/sysmacros.h>. Include this header to avoid the following sorts of build-stopping messages: qga/commands-posix.c: In function ‘dev_major_minor’: qga/commands-posix.c:656:13: error: In the GNU C Library, "major" is defined by <sys/sysmacros.h>. For historical compatibility, it is currently defined by <sys/types.h> as well, but we plan to remove this soon. To use "major", include <sys/sysmacros.h> directly. If you did not intend to use a system-defined macro "major", you should undefine it after including <sys/types.h>. [-Werror] *devmajor = major(st.st_rdev); ^~~~~~~~~~~~~~~~~~~~~~~~~~ qga/commands-posix.c:657:13: error: In the GNU C Library, "minor" is defined by <sys/sysmacros.h>. For historical compatibility, it is currently defined by <sys/types.h> as well, but we plan to remove this soon. To use "minor", include <sys/sysmacros.h> directly. If you did not intend to use a system-defined macro "minor", you should undefine it after including <sys/types.h>. [-Werror] *devminor = minor(st.st_rdev); ^~~~~~~~~~~~~~~~~~~~~~~~~~ The additional include allows the build to complete on Fedora 26 (Rawhide) with glibc version 2.24.90. Signed-off-by: Christopher Covington <cov@codeaurora.org> --- include/sysemu/os-posix.h | 1 + 1 file changed, 1 insertion(+)