Review Request 130090: Fix incorrect definition of major(3)/minor(3) macros

[Ben Cooksley]

> On May 4, 2017, 12:40 p.m., Albert Astals Cid wrote:
> > Lamarque, you broke the build.
> 
> Lamarque Souza wrote:
>     Fixed. Thanks for the quick report about the broken build and sorry for not adding all files to the commit.
> 
> Ben Cooksley wrote:
>     This patch broke the MSVC build and will be reverted shortly. 
>     Please see https://build-sandbox.kde.org/job/Frameworks%20solid%20kf5-qt5%20WindowsQt5.7/2/consoleText for the build log.
> 
> Lamarque Souza wrote:
>     Well, the simpler solution is removing all those #if, #include and #error clauses (lines 31 oto 39 of autotests/solidudisks2test.cpp), they are not required. I do not have a MSVC machine with frameworks installed to test that now. I will test that after next frameworks release.
> 
> KJ Tsanaktsidis wrote:
>     They're needed to actually find the right header file to include for these macros on the various BSD's I think. Actually I think we should just disable this test for windows, this feature is totally meaningless on windows. Another hacky option is to code in the fallback macros (including makedev) in solidudisks2test.cpp in the same way they are defined in udisksblock.cpp, which will probably make that test compile and "work". It wouldn't really be testing anything on windows though.
>     
>     The original reason I put the #error case in that test was to make sure we found out if there was some platform that was supposed to be supported that didn't have these macros in any of the places I looked for them. I guess we found out.

If you post a revised patch i'm happy to test it on the Windows CI setup.


- Ben


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/130090/#review103182
-----------------------------------------------------------


On May 4, 2017, 12:32 p.m., KJ Tsanaktsidis wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/130090/
> -----------------------------------------------------------
> 
> (Updated May 4, 2017, 12:32 p.m.)
> 
> 
> Review request for KDE Frameworks.
> 
> 
> Repository: solid
> 
> 
> Description
> -------
> 
> Previously, udesksblock.cpp was attempting to find a definition for
> major/minor on Linux in <sys/kdev_t.h> by checking Q_OS_LINUX before
> importing the header. Q_OS_LINUX is however only set when
> qsystemdetection.h is included, and the macro was being checked first.
> 
> Even had this check worked, it would still be wrong. On a modern version
> of the userspace linux-headers, <sys/kdev_t.h> includes definitions for
> major and minor that assume each is limited to 8 bits and that dev_t is
> 16 bits. This is no longer true anymore; on Linux, major numbers can be
> up to 12 bits at present and minor numbers up to 20. Calling these
> macros with dev_t values > 2^16 would give incorrect results.
> 
> Because the Q_OS_LINUX check failed, a fallback version of the macros
> were defined for use on all platforms. The code is allegedly copied from
> kdev_t.h, except it is copied from the *kernel* version of the header,
> not the userspace version. Linux internally uses a different
> representation of dev_t than it exposes to userspace - the kernelspace
> version is 20 bits of minor/12 bits of major contiguously, but the
> userspace version packs the bits in a different order to maintain
> compatability with old 16-bit device numbers. Thus, this code also does
> not work for dev_t values > 2^16.
> 
> To fix this, we add CMake rules to search for a system-provided
> definition of the major/minor macros - on various systems, these can be
> in a few different places. As a fallback, we assume old-style 16-bit
> dev_t (although I suspect that is only used for Windows, where
> major/minor numbers are pretty meaningless anyway).
> 
> 
> Diffs
> -----
> 
>   autotests/CMakeLists.txt 54adeea62b954b9169b37f1eab8fa3e215fafafa 
>   autotests/fakeUdisks2.h PRE-CREATION 
>   autotests/fakeUdisks2.cpp PRE-CREATION 
>   autotests/solidudisks2test.cpp PRE-CREATION 
>   src/solid/devices/backends/udisks2/CMakeLists.txt 34390064af29ace07cbb3470945be098cc606d04 
>   src/solid/devices/backends/udisks2/udisksblock.cpp 0622ec77fcf670a2005d34b7a6c31ca8b53a18d8 
> 
> Diff: https://git.reviewboard.kde.org/r/130090/diff/
> 
> 
> Testing
> -------
> 
> I've written a little snippet to iterate through block devices, print their major/minor number, and their device properties. It was previously incorrectly labeling all my disks with major 0 and minor == device_number (since it was using the first 20 bits for the minor). It now correctly identifies their major/minor number.
> 
> 
> Thanks,
> 
> KJ Tsanaktsidis
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20170509/cbfe5bb3/attachment.html>