GNOME Bugzilla – Bug 777030
build error where minor() and major() cant be resolved in gio/gdbusmessage.c
Last modified: 2017-05-02 08:41:26 UTC
The functions functions minor() and major() are called in gio/gdbusmessage.c but might have never been declared/defined. The functions are usually declared and defined inline by the inclusion of sys/mkdev.h or sys/sysmacros.h However this inclusion is conditional and can fail. It does for example on android when using crystax ndk. In fact it probably only succeeds if types.h (included further above) happens to include sysmacros.h as happens unconditionally on many systems. (eg android using google's ndk). So the conditional inclusion doesnt kick in anyways and is just confusing. This can lead to build errors. Worse it can lead to just warnings (implied declaration) that go unnoticed, and run time errors that are very hard to find the cause of. The call to minor()/major() should not be made if the inline definitions have not been included. The pull request here provides a possible fix: https://github.com/GNOME/glib/pull/14
Thanks for your contribution. Could you please attach the patch to fix this issue directly to Bugzilla? We cannot merge pull requests on the GitHub mirror, as it's read-only. Thanks again!
Created attachment 343144 [details] [review] patch with proposed fix Here is a patch from the commit, as requested
Any update on this? Will you apply the patch? Thanks
Review of attachment 343144 [details] [review]: Thanks for the patch. A few comments. Please also shorten the commit message subject — git commit message subjects are conventionally 80 characters long at most. How about “gdbusmessage: Don’t use major()/minor() if they’re unavailable”. ::: gio/gdbusmessage.c @@ +36,3 @@ +//----------------------------------------- +// guard against caling minor() and minor() below if not defined (as is the case with on android with crystax ndk..verified with version 10.3.2) +// note: on some systems (eg android with google ndk) the sys/types.h included above also #includes sys/sysmacros.h Drop all the `//` comment lines; they’re unnecessary (and our coding style uses /* C-style comments */ not // C++-style comments). @@ +37,3 @@ +// guard against caling minor() and minor() below if not defined (as is the case with on android with crystax ndk..verified with version 10.3.2) +// note: on some systems (eg android with google ndk) the sys/types.h included above also #includes sys/sysmacros.h +#define NO_MINOR_MAJOR_MAKEDEV_DEFS_FOUND I’d call this `MAJOR_MINOR_NOT_FOUND` for brevity and define its value to 1 (just in case someone does `#if MAJOR_MINOR_NOT_FOUND` in future).
Created attachment 350645 [details] [review] new pacth with comments removed and #define renamed as requested This fixes the bug https://bugzilla.gnome.org/show_bug.cgi?id=777030 and does so as requested by the reviewer Philip Withnall in https://bugzilla.gnome.org/show_bug.cgi?id=777030#c4
Done as requested
Review of attachment 350645 [details] [review]: This is close enough; I’ll fix it up and push it to git master. Thanks. ::: gio/gdbusmessage.c @@ +34,3 @@ #include <sys/sysmacros.h> +#else +#define MAJOR_MINOR_NOT_FOUND=1 This is not valid C. There should be a space instead of an equals.
Created attachment 350651 [details] [review] gdbusmessage: Don’t use major()/minor() if they’re unavailable fixed typo in prev patch
sorry about the typo
(In reply to declan from comment #9) > sorry about the typo Don’t worry; I already pushed a fixed version of the patch to git master as commit aefffa3.
Thank you.