Re: Review Request 130090: Fix incorrect definition of major(3)/minor(3) macros
> 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. 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. - KJ --- 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 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, 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 > >
Re: Review Request 130084: Add a pair of flags forcing fsync during copy loop
> On April 15, 2017, 8:30 a.m., David Faure wrote:
> > But doesn't this make copying much slower in the normal case? (copying onto
> > a non-removable harddisk partition).
> >
> > It sounds to me like this should be
> > 1) done internally in kio_file (no Job API for this)
> > 2) only when the destination is a removable device
>
> Oswald Buddenhagen wrote:
> i've read multiple times that fsync isn't a performance problem on modern
> file systems any more. whatever that may mean.
> limiting this to cross-device isn't really sensible imo - a) one can have
> multiple internal disks and b) even if the disk stays in, at some point the
> flushing will commence and will be a major slowdown for subsequent operations.
> in fact, this problem is bad enough that the linux kernel community
> realized it (which, in the area of disk i/o, never ceases to amaze) -
> https://lwn.net/Articles/682582/ (obvious followup question: what kernel do
> you use? this code seems to have landed in 4.10)
>
> KJ Tsanaktsidis wrote:
> I'm using kernel `Linux kj-hedt-arch 4.10.8-1-ARCH #1 SMP PREEMPT Fri Mar
> 31 16:50:19 CEST 2017 x86_64 GNU/Linux`. My understanding is that the
> patchset you're talking about will not allow synchronous reads, such as
> faulting in application code, to get stuck behind a full writeback queue, by
> limiting how much work the MM subsystem can send to the disk - not by
> throttling how fast applications can dirty the device. I've not noticed any
> problems using the disk whilst writing with or without my patch to KIO here
> but I haven't really stressed it.
>
> As to what the performance implications of fsync - I guess it depends how
> much you care about what you were planning to do with the file after you copy
> it. I implemented the "fsync if source/dest are on different filesystems"
> logic because in that case, one of the things you might be wanting to do is
> unmount the disk. If you wanted to interact with the file on the destination
> system instead, this patch would make it take (much) longer. The reason I
> implemented this with a job flag is that I was envisiging making it an option
> in Dolphin - like the "move/copy" menu when you drop, you could also get
> "copy with fsync" perhaps for this reason - we don't know what the user plans
> to do with the file afterwards.
>
> I'm happy enough to use "is the device removeable?" as a heuristic for
> "the users next desired operation on this file is probably to unmount it"
> instead and delete the Job API - this would address my use case at least. How
> do you suggest I get this information in `kio_file`? On Linux it looks like I
> can get this from sysfs `/sys/dev/block/maj:min/removeable`, but I don't know
> how to do this for other platforms and don't have them available to test.
> Would the patch be OK if I just added this for linux?
>
> David Faure wrote:
> Solid has portable API for this.
> Something like this (completely untested, I'm no Solid expert, these are
> just old notes from a TODO)
>
> const QString query = QLatin1String("[StorageAccess.accessible ==
> true]");
> const QList lst = Solid::Device::listFromQuery(query);
> iterating and then using Solid::StorageDrive::isRemovable() ||
> Solid::StorageDrive::isHotpluggable() (these checks can probably be
> integrated into the query string)
>
> KJ Tsanaktsidis wrote:
> OK - that seems reasonable enough. I guess I should be able to search
> Solid's disks for the major/minor numbers from `fstat`'s `st_dev`. Whilst
> doing this I ran into a bug with Solid, which I've proposed a patch for here:
> https://git.reviewboard.kde.org/r/130090/. If that gets OK'd I'll come back
> to finishing this off :)
OK - the dependant patch to Solid https://git.reviewboard.kde.org/r/130090/ has
been committed. I think we can try pushing this forward now?
- KJ
---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/130084/#review103048
---
On April 17, 2017, 11:27 a.m., KJ Tsanaktsidis wrote:
>
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/130084/
> ---
>
> (Updated April 17, 2017, 11:27 a.m.)
>
>
> Review request for KDE Frameworks, Oswald Buddenhagen and Thiago Macieira.
>
>
> Repository: kio
>
>
> Description
> ---
>
> When copyi
Re: Review Request 130090: Fix incorrect definition of major(3)/minor(3) macros
--- 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.) Status -- This change has been marked as submitted. Review request for KDE Frameworks. Changes --- Submitted with commit 3c1e1648a94c474c9270e203e9462285cd5d0619 by Lamarque V. Souza on behalf of Konstantinos Tsanaktsidis to branch master. Repository: solid Description --- Previously, udesksblock.cpp was attempting to find a definition for major/minor on Linux in 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, 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
Re: Review Request 130090: Fix incorrect definition of major(3)/minor(3) macros
> On April 28, 2017, 1:28 p.m., Lamarque Souza wrote: > > Ship It! > > KJ Tsanaktsidis wrote: > Great - thanks for your help and for bearing with my rusty C++! What > happens now? I'm waiting on this patch to land for another patch I submitted > a while ago: https://git.reviewboard.kde.org/r/130084/ > > Lamarque Souza wrote: > Well, if you do not have a developer account I can commit the patch for > you. In order to do that I need your real name and email address to add them > to the commit's header. Nope - I don't have a developer account. Real name is Konstantinos Tsanaktsidis <kjtsanaktsi...@gmail.com> - KJ --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/130090/#review103148 ------- On April 23, 2017, 9:56 a.m., KJ Tsanaktsidis wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/130090/ > --- > > (Updated April 23, 2017, 9:56 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 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, 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 > >
Re: Review Request 130090: Fix incorrect definition of major(3)/minor(3) macros
> On April 28, 2017, 1:28 p.m., Lamarque Souza wrote: > > Ship It! Great - thanks for your help and for bearing with my rusty C++! What happens now? I'm waiting on this patch to land for another patch I submitted a while ago: https://git.reviewboard.kde.org/r/130084/ - KJ --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/130090/#review103148 --- On April 23, 2017, 9:56 a.m., KJ Tsanaktsidis wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/130090/ > --- > > (Updated April 23, 2017, 9:56 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 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, 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 > >
Re: Review Request 130090: Fix incorrect definition of major(3)/minor(3) macros
> On April 22, 2017, 10:27 a.m., Lamarque Souza wrote: > > autotests/CMakeLists.txt, line 66 > > <https://git.reviewboard.kde.org/r/130090/diff/5/?file=494817#file494817line66> > > > > CMake's developers recommend using else() instead of > > else(). The part used to be required with cmake > > 2.6.x, that is not true with cmake 3.x that we use nowadays. > > KJ Tsanaktsidis wrote: > I'm not sure I understand here - elseif() needs to have the expression to > match to enter the elseif() block? In any case I've gone ahead and deleted > this because I worked out a cleaner way to do this using cmake generator > expressions. > > Lamarque Souza wrote: > Oh sorry, I did a mistake. I thought it wase a else(). The expression is > required for elseif(), the old code was correct. You can revert to the old > code, which require less lines of code. Sorry again. Actually it was not quite correct - it was defining `HAVE_XXX_MAJOR_MINOR=1` for when the header was found, but not `HAVE_XXX_MAJOR_MINOR=0` when it wasn't. It worked on Linux because `sys/sysmacros.h` was first, but I don't think it would work on most BSD's which have it in `sys/types.h` (the `#if HAVE_SYSMACROS_MAJOR_MINOR` check wouldn't compile). This version of it sets each `HAVE_XXX` macro to either 1 or 0 on all platforms. - KJ --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/130090/#review103087 ------- On April 23, 2017, 9:56 a.m., KJ Tsanaktsidis wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/130090/ > --- > > (Updated April 23, 2017, 9:56 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 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, 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 > >
Re: Review Request 130090: Fix incorrect definition of major(3)/minor(3) macros
> On April 22, 2017, 10:27 a.m., Lamarque Souza wrote: > > autotests/CMakeLists.txt, line 66 > > <https://git.reviewboard.kde.org/r/130090/diff/5/?file=494817#file494817line66> > > > > CMake's developers recommend using else() instead of > > else(). The part used to be required with cmake > > 2.6.x, that is not true with cmake 3.x that we use nowadays. I'm not sure I understand here - elseif() needs to have the expression to match to enter the elseif() block? In any case I've gone ahead and deleted this because I worked out a cleaner way to do this using cmake generator expressions. - KJ --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/130090/#review103087 ------- On April 23, 2017, 9:56 a.m., KJ Tsanaktsidis wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/130090/ > --- > > (Updated April 23, 2017, 9:56 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 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, 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 > >
Re: Review Request 130090: Fix incorrect definition of major(3)/minor(3) macros
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/130090/ --- (Updated April 23, 2017, 9:56 a.m.) Review request for KDE Frameworks. Changes --- * Fix definition style of const-returning functions * Replace if()..elseif()...else() CMake macros with a clearer control flow Repository: solid Description --- Previously, udesksblock.cpp was attempting to find a definition for major/minor on Linux in 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, 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 (updated) - 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
Re: Review Request 130090: Fix incorrect definition of major(3)/minor(3) macros
> On April 20, 2017, 4:19 p.m., Lamarque Souza wrote: > > autotests/fakeUdisks2.h, line 35 > > <https://git.reviewboard.kde.org/r/130090/diff/4/?file=494486#file494486line35> > > > > It is common practice to pass QString parameters QString as const &: > > > > FakeUdisks2BlockDevice(QObject *parent, const device, quint64 > > device_number); Sorry - you've probably noticed by now but my C++ is pretty rusty... (no pun intended) - KJ --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/130090/#review103071 ------- On April 22, 2017, 12:38 a.m., KJ Tsanaktsidis wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/130090/ > --- > > (Updated April 22, 2017, 12:38 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 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, 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 > >
Re: Review Request 130090: Fix incorrect definition of major(3)/minor(3) macros
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/130090/ --- (Updated April 22, 2017, 12:38 a.m.) Review request for KDE Frameworks. Changes --- Pass strings as const where possible Repository: solid Description --- Previously, udesksblock.cpp was attempting to find a definition for major/minor on Linux in 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, 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 (updated) - 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
Re: Review Request 130090: Fix incorrect definition of major(3)/minor(3) macros
> On April 19, 2017, 11:28 a.m., Lamarque Souza wrote: > > autotests/solidudisks2test.cpp, line 78 > > <https://git.reviewboard.kde.org/r/130090/diff/2/?file=494476#file494476line78> > > > > I think registering to systemBus only works if you are root or has the > > right permissions in dbus' configuration. Have you tested this unit test > > when running as normal user? Actually it's not registering to the real system bus. The `QTEST_GUILESS_MAIN_SYSTEM_DBUS` macro sets up a new session bus and sets the `DBUS_SYSTEM_BUS_ADDRESS` environment variable to point to that bus, faking the real system bus out. The test definitely works as non root. This is the same strategy used by the solidfreedesktoptest tests. - KJ --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/130090/#review103067 ------- On April 20, 2017, 8:49 a.m., KJ Tsanaktsidis wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/130090/ > --- > > (Updated April 20, 2017, 8:49 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 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, 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 > >
Re: Review Request 130090: Fix incorrect definition of major(3)/minor(3) macros
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/130090/ --- (Updated April 20, 2017, 8:49 a.m.) Review request for KDE Frameworks. Changes --- Add my name to license header Fix style of pointer declarations Repository: solid Description --- Previously, udesksblock.cpp was attempting to find a definition for major/minor on Linux in 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, 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 (updated) - 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
Re: Review Request 130090: Fix incorrect definition of major(3)/minor(3) macros
> 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 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, 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 > >
Re: Review Request 130090: Fix incorrect definition of major(3)/minor(3) macros
--- 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. Changes --- Fix trailing whitespace Repository: solid Description --- Previously, udesksblock.cpp was attempting to find a definition for major/minor on Linux in 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, 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 (updated) - 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
Re: Review Request 130090: Fix incorrect definition of major(3)/minor(3) macros
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/130090/ --- (Updated April 19, 2017, 11:03 a.m.) Review request for KDE Frameworks. Changes --- Added a test case Fixed formatting of major macro definition Repository: solid Description --- Previously, udesksblock.cpp was attempting to find a definition for major/minor on Linux in 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, 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 (updated) - 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
Re: Review Request 130090: Fix incorrect definition of major(3)/minor(3) macros
> 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 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, 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 > >
Re: Review Request 130084: Add a pair of flags forcing fsync during copy loop
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/130084/ --- (Updated April 17, 2017, 11:27 a.m.) Review request for KDE Frameworks, Oswald Buddenhagen and Thiago Macieira. Changes --- Removed the Job API changes; fsync is now always used when the target is on removeable media Repository: kio Description --- When copying a large-ish file (~1-2GB) from very fast storage to very slow storage (e.g. an NVME SSD to a cheap USB flash drive) on a machine with lots of RAM, Dolphin displays a progress bar which finishes in a fraction of a second (i.e. as fast as it takes to read the source file into the Linux page cache). Unmounting the drive then of course takes a long time, with only an indeterminate spinner. This patch adds an option to force fsync during copy jobs, so that the copy progress bar measures how long it will take to actually copy the file to the destination. I've added two flags - Fsync and FsyncCrossFilesystem - to the JobEnum flag. The former will cause all copy operations to fsync during the copy loop, whilst the latter will only fsync copies that are across different filesystems. If this patch gets OK'd, I have another patch which adds support for this into the appropriate places in Dolphin. I would think that at least FsyncCrossFilesystem should be the default, but Fsync always might be a little heavy handed. At the least fsync'ing cross-filesystem copies ensures that the unmount won't take forever. Diffs (updated) - src/ioslaves/file/CMakeLists.txt b9132ced9d4a08b2cf9f9bbbaa3bd43f026c6469 src/ioslaves/file/ConfigureChecks.cmake 5a83d1b9fbe90c851c774e3b467468d93b5a2bd4 src/ioslaves/file/config-kioslave-file.h.cmake 372f79d01ad4597aae0b2ae62627648fe7680b64 src/ioslaves/file/file_unix.cpp 3c1b9927e3dd2d0134f77caec6e6b24a0356d26f Diff: https://git.reviewboard.kde.org/r/130084/diff/ Testing --- Tested the patch with KDE/Dolphin on Arch Linux, which is version 5.32.0. The diff applies cleanly to master so I assume there shouldn't be any issues there, but I've not actually checked that. As advertised, copying a file to USB flash storage now displays an accurate progress bar. I experimented with how often fsync should be called on my hardware, and I found calling it every ~1M copied caused no decrease in copy performance whilst still providing accurate progress info. That is the setting I've gone with in this patch. I'm open to suggestions on how this could be tuned better though. Thanks, KJ Tsanaktsidis
Re: Review Request 130084: Add a pair of flags forcing fsync during copy loop
> On April 15, 2017, 8:30 a.m., David Faure wrote:
> > But doesn't this make copying much slower in the normal case? (copying onto
> > a non-removable harddisk partition).
> >
> > It sounds to me like this should be
> > 1) done internally in kio_file (no Job API for this)
> > 2) only when the destination is a removable device
>
> Oswald Buddenhagen wrote:
> i've read multiple times that fsync isn't a performance problem on modern
> file systems any more. whatever that may mean.
> limiting this to cross-device isn't really sensible imo - a) one can have
> multiple internal disks and b) even if the disk stays in, at some point the
> flushing will commence and will be a major slowdown for subsequent operations.
> in fact, this problem is bad enough that the linux kernel community
> realized it (which, in the area of disk i/o, never ceases to amaze) -
> https://lwn.net/Articles/682582/ (obvious followup question: what kernel do
> you use? this code seems to have landed in 4.10)
>
> KJ Tsanaktsidis wrote:
> I'm using kernel `Linux kj-hedt-arch 4.10.8-1-ARCH #1 SMP PREEMPT Fri Mar
> 31 16:50:19 CEST 2017 x86_64 GNU/Linux`. My understanding is that the
> patchset you're talking about will not allow synchronous reads, such as
> faulting in application code, to get stuck behind a full writeback queue, by
> limiting how much work the MM subsystem can send to the disk - not by
> throttling how fast applications can dirty the device. I've not noticed any
> problems using the disk whilst writing with or without my patch to KIO here
> but I haven't really stressed it.
>
> As to what the performance implications of fsync - I guess it depends how
> much you care about what you were planning to do with the file after you copy
> it. I implemented the "fsync if source/dest are on different filesystems"
> logic because in that case, one of the things you might be wanting to do is
> unmount the disk. If you wanted to interact with the file on the destination
> system instead, this patch would make it take (much) longer. The reason I
> implemented this with a job flag is that I was envisiging making it an option
> in Dolphin - like the "move/copy" menu when you drop, you could also get
> "copy with fsync" perhaps for this reason - we don't know what the user plans
> to do with the file afterwards.
>
> I'm happy enough to use "is the device removeable?" as a heuristic for
> "the users next desired operation on this file is probably to unmount it"
> instead and delete the Job API - this would address my use case at least. How
> do you suggest I get this information in `kio_file`? On Linux it looks like I
> can get this from sysfs `/sys/dev/block/maj:min/removeable`, but I don't know
> how to do this for other platforms and don't have them available to test.
> Would the patch be OK if I just added this for linux?
>
> David Faure wrote:
> Solid has portable API for this.
> Something like this (completely untested, I'm no Solid expert, these are
> just old notes from a TODO)
>
> const QString query = QLatin1String("[StorageAccess.accessible ==
> true]");
> const QList lst = Solid::Device::listFromQuery(query);
> iterating and then using Solid::StorageDrive::isRemovable() ||
> Solid::StorageDrive::isHotpluggable() (these checks can probably be
> integrated into the query string)
OK - that seems reasonable enough. I guess I should be able to search Solid's
disks for the major/minor numbers from `fstat`'s `st_dev`. Whilst doing this I
ran into a bug with Solid, which I've proposed a patch for here:
https://git.reviewboard.kde.org/r/130090/. If that gets OK'd I'll come back to
finishing this off :)
- KJ
---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/130084/#review103048
---
On April 15, 2017, 8:28 a.m., KJ Tsanaktsidis wrote:
>
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/130084/
> ---
>
> (Updated April 15, 2017, 8:28 a.m.)
>
>
> Review request for KDE Frameworks, Oswald Buddenhagen and Thiago Macieira.
>
>
> Repository: kio
>
>
> Description
> ---
>
> When copying a large-ish file (~1-2GB) from very fast storage to very slow
> storage (e.g. an NVME SSD to a cheap USB flash drive) on a machine with lots
> of RAM, Dolphin displays a progress bar which finishes
Review Request 130090: Fix incorrect definition of major(3)/minor(3) macros
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/130090/ --- Review request for KDE Frameworks. Repository: solid Description --- Previously, udesksblock.cpp was attempting to find a definition for major/minor on Linux in 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, 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
Re: Review Request 130084: Add a pair of flags forcing fsync during copy loop
> On April 15, 2017, 8:30 a.m., David Faure wrote: > > But doesn't this make copying much slower in the normal case? (copying onto > > a non-removable harddisk partition). > > > > It sounds to me like this should be > > 1) done internally in kio_file (no Job API for this) > > 2) only when the destination is a removable device > > Oswald Buddenhagen wrote: > i've read multiple times that fsync isn't a performance problem on modern > file systems any more. whatever that may mean. > limiting this to cross-device isn't really sensible imo - a) one can have > multiple internal disks and b) even if the disk stays in, at some point the > flushing will commence and will be a major slowdown for subsequent operations. > in fact, this problem is bad enough that the linux kernel community > realized it (which, in the area of disk i/o, never ceases to amaze) - > https://lwn.net/Articles/682582/ (obvious followup question: what kernel do > you use? this code seems to have landed in 4.10) I'm using kernel `Linux kj-hedt-arch 4.10.8-1-ARCH #1 SMP PREEMPT Fri Mar 31 16:50:19 CEST 2017 x86_64 GNU/Linux`. My understanding is that the patchset you're talking about will not allow synchronous reads, such as faulting in application code, to get stuck behind a full writeback queue, by limiting how much work the MM subsystem can send to the disk - not by throttling how fast applications can dirty the device. I've not noticed any problems using the disk whilst writing with or without my patch to KIO here but I haven't really stressed it. As to what the performance implications of fsync - I guess it depends how much you care about what you were planning to do with the file after you copy it. I implemented the "fsync if source/dest are on different filesystems" logic because in that case, one of the things you might be wanting to do is unmount the disk. If you wanted to interact with the file on the destination system instead, this patch would make it take (much) longer. The reason I implemented this with a job flag is that I was envisiging making it an option in Dolphin - like the "move/copy" menu when you drop, you could also get "copy with fsync" perhaps for this reason - we don't know what the user plans to do with the file afterwards. I'm happy enough to use "is the device removeable?" as a heuristic for "the users next desired operation on this file is probably to unmount it" instead and delete the Job API - this would address my use case at least. How do you suggest I get this information in `kio_file`? On Linux it looks like I can get this from sysfs `/sys/dev/block/maj:min/removeable`, but I don't know how to do this for other platforms and don't have them available to test. Would the patch be OK if I just added this for linux? - KJ --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/130084/#review103048 --- On April 15, 2017, 8:28 a.m., KJ Tsanaktsidis wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/130084/ > --- > > (Updated April 15, 2017, 8:28 a.m.) > > > Review request for KDE Frameworks, Oswald Buddenhagen and Thiago Macieira. > > > Repository: kio > > > Description > --- > > When copying a large-ish file (~1-2GB) from very fast storage to very slow > storage (e.g. an NVME SSD to a cheap USB flash drive) on a machine with lots > of RAM, Dolphin displays a progress bar which finishes in a fraction of a > second (i.e. as fast as it takes to read the source file into the Linux page > cache). Unmounting the drive then of course takes a long time, with only an > indeterminate spinner. > > This patch adds an option to force fsync during copy jobs, so that the copy > progress bar measures how long it will take to actually copy the file to the > destination. > > I've added two flags - Fsync and FsyncCrossFilesystem - to the JobEnum flag. > The former will cause all copy operations to fsync during the copy loop, > whilst the latter will only fsync copies that are across different > filesystems. > > If this patch gets OK'd, I have another patch which adds support for this > into the appropriate places in Dolphin. I would think that at least > FsyncCrossFilesystem should be the default, but Fsync always might be a > little heavy handed. At the least fsync'ing cross-filesystem copies ensures > that the unmount won't take forever. > > >
Re: Review Request 130084: Add a pair of flags forcing fsync during copy loop
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/130084/ --- (Updated April 12, 2017, 11:39 a.m.) Review request for KDE Frameworks. Changes --- Add a pair of flags forcing fsync during copy loop Repository: kio Description --- When copying a large-ish file (~1-2GB) from very fast storage to very slow storage (e.g. an NVME SSD to a cheap USB flash drive) on a machine with lots of RAM, Dolphin displays a progress bar which finishes in a fraction of a second (i.e. as fast as it takes to read the source file into the Linux page cache). Unmounting the drive then of course takes a long time, with only an indeterminate spinner. This patch adds an option to force fsync during copy jobs, so that the copy progress bar measures how long it will take to actually copy the file to the destination. I've added two flags - Fsync and FsyncCrossFilesystem - to the JobEnum flag. The former will cause all copy operations to fsync during the copy loop, whilst the latter will only fsync copies that are across different filesystems. If this patch gets OK'd, I have another patch which adds support for this into the appropriate places in Dolphin. I would think that at least FsyncCrossFilesystem should be the default, but Fsync always might be a little heavy handed. At the least fsync'ing cross-filesystem copies ensures that the unmount won't take forever. Diffs (updated) - src/core/copyjob.cpp 7c02cb50d7e9c11bbcd9264832357f3fd6dc8c16 src/core/filecopyjob.cpp 301b7039158b7dc537b9004c14845b3d1d60f8eb src/core/job_base.h 0be9629f42277afc5f72d00d0cae5c9c1cd2b8bc src/core/slavebase.cpp 3778df813b8568657a2cbd9412c1244f94696a0c src/ioslaves/file/file_unix.cpp 3c1b9927e3dd2d0134f77caec6e6b24a0356d26f Diff: https://git.reviewboard.kde.org/r/130084/diff/ Testing --- Tested the patch with KDE/Dolphin on Arch Linux, which is version 5.32.0. The diff applies cleanly to master so I assume there shouldn't be any issues there, but I've not actually checked that. As advertised, copying a file to USB flash storage now displays an accurate progress bar. I experimented with how often fsync should be called on my hardware, and I found calling it every ~1M copied caused no decrease in copy performance whilst still providing accurate progress info. That is the setting I've gone with in this patch. I'm open to suggestions on how this could be tuned better though. Thanks, KJ Tsanaktsidis
Review Request 130084: Add a pair of flags forcing fsync during copy loop
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/130084/ --- Review request for KDE Frameworks. Repository: kio Description --- When copying a large-ish file (~1-2GB) from very fast storage to very slow storage (e.g. an NVME SSD to a cheap USB flash drive) on a machine with lots of RAM, Dolphin displays a progress bar which finishes in a fraction of a second (i.e. as fast as it takes to read the source file into the Linux page cache). Unmounting the drive then of course takes a long time, with only an indeterminate spinner. This patch adds an option to force fsync during copy jobs, so that the copy progress bar measures how long it will take to actually copy the file to the destination. I've added two flags - Fsync and FsyncCrossFilesystem - to the JobEnum flag. The former will cause all copy operations to fsync during the copy loop, whilst the latter will only fsync copies that are across different filesystems. If this patch gets OK'd, I have another patch which adds support for this into the appropriate places in Dolphin. I would think that at least FsyncCrossFilesystem should be the default, but Fsync always might be a little heavy handed. At the least fsync'ing cross-filesystem copies ensures that the unmount won't take forever. Diffs - Diff: https://git.reviewboard.kde.org/r/130084/diff/ Testing --- Tested the patch with KDE/Dolphin on Arch Linux, which is version 5.32.0. The diff applies cleanly to master so I assume there shouldn't be any issues there, but I've not actually checked that. As advertised, copying a file to USB flash storage now displays an accurate progress bar. I experimented with how often fsync should be called on my hardware, and I found calling it every ~1M copied caused no decrease in copy performance whilst still providing accurate progress info. That is the setting I've gone with in this patch. I'm open to suggestions on how this could be tuned better though. Thanks, KJ Tsanaktsidis
