Re: Review Request 123479: Adapt to Qt 5.6 changes and prevent nullbytes in QStrings.
On April 23, 2015, 11:33 p.m., Alex Richardson wrote:
src/lib/io/kdirwatch.cpp, line 303
https://git.reviewboard.kde.org/r/123479/diff/2/?file=362725#file362725line303
Why this manual loop instead of strlen()? Does that mean that null
characters in the middle are valid? Or, more likely, this reverse loop is
an optimization? If so I would add a comment since it wasn't obvious to me
straight away.
Alex Richardson wrote:
Maybe this here is easier to read?
if (cpath.endsWith('\0')) {
cpath.resize(cpath.lastIndexOf('\0'));
}
strlen would also iterate over the beginning which is uninteresting. and your
cpath code above is wrong (should be indexOf, not lastIndexOf) and also much
slower as it adds another unneccessary temporary allocation. What my loop does
it skip all trailing nullbytes. I can add that as a comment if you like.
- Milian
---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/123479/#review79415
---
On April 23, 2015, 5:19 p.m., Milian Wolff wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/123479/
---
(Updated April 23, 2015, 5:19 p.m.)
Review request for KDE Frameworks, David Faure, Marc Mutz, and Volker Krause.
Repository: kcoreaddons
Description
---
The len in inotify_event includes nulls of the name. To prevent
them from being included in the QString/QByteArray we must filter
them manually with a recent Qt 5 dev build now. See also:
https://codereview.qt-project.org/#/c/106473/
REVIEW: 123479
Diffs
-
autotests/kdirwatch_unittest.cpp 83b5b410e987fb45a08f8251ec65496c8074b077
src/lib/io/kdirwatch.cpp a75fae012c59021996b46845db2e97abb5526930
Diff: https://git.reviewboard.kde.org/r/123479/diff/
Testing
---
I used the test and looked at the output and also ran it against a patched
qtbase with this:
https://paste.kde.org/pmoue241d
Thanks,
Milian Wolff
___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 123479: Adapt to Qt 5.6 changes and prevent nullbytes in QStrings.
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/123479/ --- (Updated April 24, 2015, 12:06 p.m.) Status -- This change has been marked as submitted. Review request for KDE Frameworks, David Faure, Marc Mutz, and Volker Krause. Changes --- Submitted with commit ff964c0c42202228348213e7d5c10f5611a9ee54 by Milian Wolff to branch master. Repository: kcoreaddons Description --- The len in inotify_event includes nulls of the name. To prevent them from being included in the QString/QByteArray we must filter them manually with a recent Qt 5 dev build now. See also: https://codereview.qt-project.org/#/c/106473/ REVIEW: 123479 Diffs - autotests/kdirwatch_unittest.cpp 83b5b410e987fb45a08f8251ec65496c8074b077 src/lib/io/kdirwatch.cpp a75fae012c59021996b46845db2e97abb5526930 Diff: https://git.reviewboard.kde.org/r/123479/diff/ Testing --- I used the test and looked at the output and also ran it against a patched qtbase with this: https://paste.kde.org/pmoue241d Thanks, Milian Wolff ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 123479: Adapt to Qt 5.6 changes and prevent nullbytes in QStrings.
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/123479/ --- (Updated April 23, 2015, 4:50 p.m.) Review request for KDE Frameworks, David Faure, Marc Mutz, and Volker Krause. Repository: kcoreaddons Description --- The len in inotify_event includes nulls of the name. To prevent them from being included in the QString/QByteArray we must filter them manually with a recent Qt 5 dev build now. See also: https://codereview.qt-project.org/#/c/106473/ REVIEW: 123479 Diffs - src/lib/io/kdirwatch.cpp a75fae012c59021996b46845db2e97abb5526930 Diff: https://git.reviewboard.kde.org/r/123479/diff/ Testing (updated) --- I used the test and looked at the output and also ran it against a patched qtbase with this: https://paste.kde.org/pmoue241d Thanks, Milian Wolff ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 123479: Adapt to Qt 5.6 changes and prevent nullbytes in QStrings.
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/123479/#review79399 --- How are kf5 releases handled btw? Do I need to push this into some branch? There are none in kcoreaddons. This patch here should probably be put into a patch release (if we do this for kf5). - Milian Wolff On April 23, 2015, 4:50 p.m., Milian Wolff wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/123479/ --- (Updated April 23, 2015, 4:50 p.m.) Review request for KDE Frameworks, David Faure, Marc Mutz, and Volker Krause. Repository: kcoreaddons Description --- The len in inotify_event includes nulls of the name. To prevent them from being included in the QString/QByteArray we must filter them manually with a recent Qt 5 dev build now. See also: https://codereview.qt-project.org/#/c/106473/ REVIEW: 123479 Diffs - src/lib/io/kdirwatch.cpp a75fae012c59021996b46845db2e97abb5526930 Diff: https://git.reviewboard.kde.org/r/123479/diff/ Testing --- I used the test and looked at the output and also ran it against a patched qtbase with this: https://paste.kde.org/pmoue241d Thanks, Milian Wolff ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 123479: Adapt to Qt 5.6 changes and prevent nullbytes in QStrings.
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/123479/#review79400 --- LGTM, but I think having/modifying a unit test for this change would be useful. - Matthew Dawson On April 23, 2015, 12:50 p.m., Milian Wolff wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/123479/ --- (Updated April 23, 2015, 12:50 p.m.) Review request for KDE Frameworks, David Faure, Marc Mutz, and Volker Krause. Repository: kcoreaddons Description --- The len in inotify_event includes nulls of the name. To prevent them from being included in the QString/QByteArray we must filter them manually with a recent Qt 5 dev build now. See also: https://codereview.qt-project.org/#/c/106473/ REVIEW: 123479 Diffs - src/lib/io/kdirwatch.cpp a75fae012c59021996b46845db2e97abb5526930 Diff: https://git.reviewboard.kde.org/r/123479/diff/ Testing --- I used the test and looked at the output and also ran it against a patched qtbase with this: https://paste.kde.org/pmoue241d Thanks, Milian Wolff ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 123479: Adapt to Qt 5.6 changes and prevent nullbytes in QStrings.
On April 23, 2015, 6:55 p.m., Milian Wolff wrote: How are kf5 releases handled btw? Do I need to push this into some branch? There are none in kcoreaddons. This patch here should probably be put into a patch release (if we do this for kf5). The current schedule is one release every month, no patch releases, master always build-able. - Luigi --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/123479/#review79399 --- On April 23, 2015, 6:50 p.m., Milian Wolff wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/123479/ --- (Updated April 23, 2015, 6:50 p.m.) Review request for KDE Frameworks, David Faure, Marc Mutz, and Volker Krause. Repository: kcoreaddons Description --- The len in inotify_event includes nulls of the name. To prevent them from being included in the QString/QByteArray we must filter them manually with a recent Qt 5 dev build now. See also: https://codereview.qt-project.org/#/c/106473/ REVIEW: 123479 Diffs - src/lib/io/kdirwatch.cpp a75fae012c59021996b46845db2e97abb5526930 Diff: https://git.reviewboard.kde.org/r/123479/diff/ Testing --- I used the test and looked at the output and also ran it against a patched qtbase with this: https://paste.kde.org/pmoue241d Thanks, Milian Wolff ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 123479: Adapt to Qt 5.6 changes and prevent nullbytes in QStrings.
On April 23, 2015, 12:55 p.m., Milian Wolff wrote: How are kf5 releases handled btw? Do I need to push this into some branch? There are none in kcoreaddons. This patch here should probably be put into a patch release (if we do this for kf5). Luigi Toscano wrote: The current schedule is one release every month, no patch releases, master always build-able. KF5 has monthly releases from the master branch, and no patch release unless something critical shows up. So this should just get pushed to master once the ship it is given. - Matthew --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/123479/#review79399 --- On April 23, 2015, 12:50 p.m., Milian Wolff wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/123479/ --- (Updated April 23, 2015, 12:50 p.m.) Review request for KDE Frameworks, David Faure, Marc Mutz, and Volker Krause. Repository: kcoreaddons Description --- The len in inotify_event includes nulls of the name. To prevent them from being included in the QString/QByteArray we must filter them manually with a recent Qt 5 dev build now. See also: https://codereview.qt-project.org/#/c/106473/ REVIEW: 123479 Diffs - src/lib/io/kdirwatch.cpp a75fae012c59021996b46845db2e97abb5526930 Diff: https://git.reviewboard.kde.org/r/123479/diff/ Testing --- I used the test and looked at the output and also ran it against a patched qtbase with this: https://paste.kde.org/pmoue241d Thanks, Milian Wolff ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 123479: Adapt to Qt 5.6 changes and prevent nullbytes in QStrings.
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/123479/#review79404 --- +1 from me, but I don't want to hit ship it without hearing from someone else. - Matthew Dawson On April 23, 2015, 1:19 p.m., Milian Wolff wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/123479/ --- (Updated April 23, 2015, 1:19 p.m.) Review request for KDE Frameworks, David Faure, Marc Mutz, and Volker Krause. Repository: kcoreaddons Description --- The len in inotify_event includes nulls of the name. To prevent them from being included in the QString/QByteArray we must filter them manually with a recent Qt 5 dev build now. See also: https://codereview.qt-project.org/#/c/106473/ REVIEW: 123479 Diffs - autotests/kdirwatch_unittest.cpp 83b5b410e987fb45a08f8251ec65496c8074b077 src/lib/io/kdirwatch.cpp a75fae012c59021996b46845db2e97abb5526930 Diff: https://git.reviewboard.kde.org/r/123479/diff/ Testing --- I used the test and looked at the output and also ran it against a patched qtbase with this: https://paste.kde.org/pmoue241d Thanks, Milian Wolff ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 123479: Adapt to Qt 5.6 changes and prevent nullbytes in QStrings.
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/123479/ --- (Updated April 23, 2015, 5:19 p.m.) Review request for KDE Frameworks, David Faure, Marc Mutz, and Volker Krause. Changes --- extended an existing test to verify this bug is fixed Repository: kcoreaddons Description --- The len in inotify_event includes nulls of the name. To prevent them from being included in the QString/QByteArray we must filter them manually with a recent Qt 5 dev build now. See also: https://codereview.qt-project.org/#/c/106473/ REVIEW: 123479 Diffs (updated) - autotests/kdirwatch_unittest.cpp 83b5b410e987fb45a08f8251ec65496c8074b077 src/lib/io/kdirwatch.cpp a75fae012c59021996b46845db2e97abb5526930 Diff: https://git.reviewboard.kde.org/r/123479/diff/ Testing --- I used the test and looked at the output and also ran it against a patched qtbase with this: https://paste.kde.org/pmoue241d Thanks, Milian Wolff ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 123479: Adapt to Qt 5.6 changes and prevent nullbytes in QStrings.
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/123479/#review79411 --- Ship it! Ship It! - Aleix Pol Gonzalez On April 23, 2015, 7:19 p.m., Milian Wolff wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/123479/ --- (Updated April 23, 2015, 7:19 p.m.) Review request for KDE Frameworks, David Faure, Marc Mutz, and Volker Krause. Repository: kcoreaddons Description --- The len in inotify_event includes nulls of the name. To prevent them from being included in the QString/QByteArray we must filter them manually with a recent Qt 5 dev build now. See also: https://codereview.qt-project.org/#/c/106473/ REVIEW: 123479 Diffs - autotests/kdirwatch_unittest.cpp 83b5b410e987fb45a08f8251ec65496c8074b077 src/lib/io/kdirwatch.cpp a75fae012c59021996b46845db2e97abb5526930 Diff: https://git.reviewboard.kde.org/r/123479/diff/ Testing --- I used the test and looked at the output and also ran it against a patched qtbase with this: https://paste.kde.org/pmoue241d Thanks, Milian Wolff ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 123479: Adapt to Qt 5.6 changes and prevent nullbytes in QStrings.
On April 24, 2015, 12:33 a.m., Alex Richardson wrote:
src/lib/io/kdirwatch.cpp, line 303
https://git.reviewboard.kde.org/r/123479/diff/2/?file=362725#file362725line303
Why this manual loop instead of strlen()? Does that mean that null
characters in the middle are valid? Or, more likely, this reverse loop is
an optimization? If so I would add a comment since it wasn't obvious to me
straight away.
Maybe this here is easier to read?
if (cpath.endsWith('\0')) {
cpath.resize(cpath.lastIndexOf('\0'));
}
- Alex
---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/123479/#review79415
---
On April 23, 2015, 6:19 p.m., Milian Wolff wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/123479/
---
(Updated April 23, 2015, 6:19 p.m.)
Review request for KDE Frameworks, David Faure, Marc Mutz, and Volker Krause.
Repository: kcoreaddons
Description
---
The len in inotify_event includes nulls of the name. To prevent
them from being included in the QString/QByteArray we must filter
them manually with a recent Qt 5 dev build now. See also:
https://codereview.qt-project.org/#/c/106473/
REVIEW: 123479
Diffs
-
autotests/kdirwatch_unittest.cpp 83b5b410e987fb45a08f8251ec65496c8074b077
src/lib/io/kdirwatch.cpp a75fae012c59021996b46845db2e97abb5526930
Diff: https://git.reviewboard.kde.org/r/123479/diff/
Testing
---
I used the test and looked at the output and also ran it against a patched
qtbase with this:
https://paste.kde.org/pmoue241d
Thanks,
Milian Wolff
___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 123479: Adapt to Qt 5.6 changes and prevent nullbytes in QStrings.
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/123479/#review79415 --- src/lib/io/kdirwatch.cpp (line 303) https://git.reviewboard.kde.org/r/123479/#comment54265 Why this manual loop instead of strlen()? Does that mean that null characters in the middle are valid? Or, more likely, this reverse loop is an optimization? If so I would add a comment since it wasn't obvious to me straight away. - Alex Richardson On April 23, 2015, 6:19 p.m., Milian Wolff wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/123479/ --- (Updated April 23, 2015, 6:19 p.m.) Review request for KDE Frameworks, David Faure, Marc Mutz, and Volker Krause. Repository: kcoreaddons Description --- The len in inotify_event includes nulls of the name. To prevent them from being included in the QString/QByteArray we must filter them manually with a recent Qt 5 dev build now. See also: https://codereview.qt-project.org/#/c/106473/ REVIEW: 123479 Diffs - autotests/kdirwatch_unittest.cpp 83b5b410e987fb45a08f8251ec65496c8074b077 src/lib/io/kdirwatch.cpp a75fae012c59021996b46845db2e97abb5526930 Diff: https://git.reviewboard.kde.org/r/123479/diff/ Testing --- I used the test and looked at the output and also ran it against a patched qtbase with this: https://paste.kde.org/pmoue241d Thanks, Milian Wolff ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
