> On April 18, 2017, 9:45 p.m., Lamarque Souza wrote: > > src/solid/devices/backends/udisks2/udisksblock.cpp, line 31 > > <https://git.reviewboard.kde.org/r/130090/diff/1/?file=494454#file494454line31> > > > > Usually we use uppper case for macros. That would also save some lines > > in this patch. > > > > nitpik: add spaces around >>, like you did for & in the line below. > > KJ Tsanaktsidis wrote: > Thanks for the review. > > * I would use upper case, but the macro is defined in `sys/sysmacros.h` > and `sys/types.h` in lowercase. See > https://code.woboq.org/userspace/glibc/misc/sys/sysmacros.h.html. > * I'm happy enough to fix the formatting issue with `>>`. > * I'm also halfway through writing a testcase for this - the udisks2 > stuff is untested so I figured it would be worth going through the motion and > setting up the test framework for it even just for this to make adding > further test coverage useful. I'll add that to this patch when I finish it > either tonight or tomorrow.
OK - I've addressed the formatting issue and added a testcase. Also the link in my comment above is wrong because it has a trailing full-stop - should be https://code.woboq.org/userspace/glibc/misc/sys/sysmacros.h.html - KJ ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/130090/#review103063 ----------------------------------------------------------- On April 19, 2017, 11:05 a.m., KJ Tsanaktsidis wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/130090/ > ----------------------------------------------------------- > > (Updated April 19, 2017, 11:05 a.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 > >
