> 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.
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. - KJ ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/130090/#review103063 ----------------------------------------------------------- On April 17, 2017, 8:42 a.m., KJ Tsanaktsidis wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/130090/ > ----------------------------------------------------------- > > (Updated April 17, 2017, 8:42 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 > ----- > > 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 > >
