Re: Review Request 125515: Preserve relative link targets when copying symlinks.
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/125515/ --- (Updated Oct. 24, 2015, 6:02 p.m.) Status -- This change has been marked as submitted. Review request for KDE Frameworks. Changes --- Submitted with commit f2c96d1550f22600f1b6dee74851c25a3f733ef4 by David Faure to branch master. Bugs: 352927 https://bugs.kde.org/show_bug.cgi?id=352927 Repository: kio Description --- BUG: 352927 REVIEW: 125515 Change-Id: I7d3c988da32cae9d14750c8adb9ca5af6d140572 Diffs - autotests/jobtest.h ef8c3e111ec647cc59c5a9715ab3220ce1651c9e autotests/jobtest.cpp c24bcead70f78f2bec3b938819fb2fa842e937d5 src/ioslaves/file/file.cpp a0a533c957628b00eff175a949685d4497c5f095 Diff: https://git.reviewboard.kde.org/r/125515/diff/ Testing --- 2 unit tests added Thanks, David Faure ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 125515: Preserve relative link targets when copying symlinks.
> On Oct. 22, 2015, 4:32 p.m., Frank Reininghaus wrote: > > 4096 bytes looks reasonable. I think I would still find a fail-safe > > solution with a dynamically increasing buffer prettier, but it's so > > extremely unlikely that this will ever cause problems that it's not worth > > arguing about it :-) > > Stefan Brüns wrote: > The needed length is available from lstat(.., &stat), stat.st_size. Oh, that's perfect, we already just do an lstat() just before we need the size... Thanks I'll do that and push. (I'll leave 4096 in the unittest ;) - David --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/125515/#review87274 --- On Oct. 10, 2015, 3:29 p.m., David Faure wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/125515/ > --- > > (Updated Oct. 10, 2015, 3:29 p.m.) > > > Review request for KDE Frameworks. > > > Bugs: 352927 > https://bugs.kde.org/show_bug.cgi?id=352927 > > > Repository: kio > > > Description > --- > > BUG: 352927 > REVIEW: 125515 > Change-Id: I7d3c988da32cae9d14750c8adb9ca5af6d140572 > > > Diffs > - > > autotests/jobtest.h ef8c3e111ec647cc59c5a9715ab3220ce1651c9e > autotests/jobtest.cpp c24bcead70f78f2bec3b938819fb2fa842e937d5 > src/ioslaves/file/file.cpp a0a533c957628b00eff175a949685d4497c5f095 > > Diff: https://git.reviewboard.kde.org/r/125515/diff/ > > > Testing > --- > > 2 unit tests added > > > Thanks, > > David Faure > > ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 125515: Preserve relative link targets when copying symlinks.
> On Oct. 22, 2015, 4:32 p.m., Frank Reininghaus wrote: > > 4096 bytes looks reasonable. I think I would still find a fail-safe > > solution with a dynamically increasing buffer prettier, but it's so > > extremely unlikely that this will ever cause problems that it's not worth > > arguing about it :-) The needed length is available from lstat(.., &stat), stat.st_size. - Stefan --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/125515/#review87274 --- On Oct. 10, 2015, 3:29 p.m., David Faure wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/125515/ > --- > > (Updated Oct. 10, 2015, 3:29 p.m.) > > > Review request for KDE Frameworks. > > > Bugs: 352927 > https://bugs.kde.org/show_bug.cgi?id=352927 > > > Repository: kio > > > Description > --- > > BUG: 352927 > REVIEW: 125515 > Change-Id: I7d3c988da32cae9d14750c8adb9ca5af6d140572 > > > Diffs > - > > autotests/jobtest.h ef8c3e111ec647cc59c5a9715ab3220ce1651c9e > autotests/jobtest.cpp c24bcead70f78f2bec3b938819fb2fa842e937d5 > src/ioslaves/file/file.cpp a0a533c957628b00eff175a949685d4497c5f095 > > Diff: https://git.reviewboard.kde.org/r/125515/diff/ > > > Testing > --- > > 2 unit tests added > > > Thanks, > > David Faure > > ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 125515: Preserve relative link targets when copying symlinks.
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/125515/#review87274 --- Ship it! 4096 bytes looks reasonable. I think I would still find a fail-safe solution with a dynamically increasing buffer prettier, but it's so extremely unlikely that this will ever cause problems that it's not worth arguing about it :-) - Frank Reininghaus On Okt. 10, 2015, 3:29 nachm., David Faure wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/125515/ > --- > > (Updated Okt. 10, 2015, 3:29 nachm.) > > > Review request for KDE Frameworks. > > > Bugs: 352927 > https://bugs.kde.org/show_bug.cgi?id=352927 > > > Repository: kio > > > Description > --- > > BUG: 352927 > REVIEW: 125515 > Change-Id: I7d3c988da32cae9d14750c8adb9ca5af6d140572 > > > Diffs > - > > autotests/jobtest.h ef8c3e111ec647cc59c5a9715ab3220ce1651c9e > autotests/jobtest.cpp c24bcead70f78f2bec3b938819fb2fa842e937d5 > src/ioslaves/file/file.cpp a0a533c957628b00eff175a949685d4497c5f095 > > Diff: https://git.reviewboard.kde.org/r/125515/diff/ > > > Testing > --- > > 2 unit tests added > > > Thanks, > > David Faure > > ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 125515: Preserve relative link targets when copying symlinks.
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/125515/ --- (Updated Oct. 10, 2015, 3:29 p.m.) Review request for KDE Frameworks. Changes --- Increase symlink target max len to 4096 Bugs: 352927 https://bugs.kde.org/show_bug.cgi?id=352927 Repository: kio Description (updated) --- BUG: 352927 REVIEW: 125515 Change-Id: I7d3c988da32cae9d14750c8adb9ca5af6d140572 Diffs (updated) - autotests/jobtest.h ef8c3e111ec647cc59c5a9715ab3220ce1651c9e autotests/jobtest.cpp c24bcead70f78f2bec3b938819fb2fa842e937d5 src/ioslaves/file/file.cpp a0a533c957628b00eff175a949685d4497c5f095 Diff: https://git.reviewboard.kde.org/r/125515/diff/ Testing --- 2 unit tests added Thanks, David Faure ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 125515: Preserve relative link targets when copying symlinks.
> On Oct. 4, 2015, 5:45 p.m., Frank Reininghaus wrote: > > Looks good! The only thing that I'm not sure about is whether 1000 bytes > > are guaranteed to be enough. Some quick Googling tells me that path lengths > > of 4096 are possible. Maybe we could allocate a temporary large array on > > the heap if the readlink call fails with the 1000 byte buffer on the stack? > > Xuetian Weng wrote: > Just FYI, even PATH_MAX may not exists on some system (AFAIK, hurd, > allowed by posix). > And though st.st_size is mentioned in readlink man page, it may not > return useful value. > > > More information > http://stackoverflow.com/questions/9385386/howto-use-readlink-with-dynamic-memory-allocation > > David Faure wrote: > Just for info, I basically restored this code from kdelibs4 :) > But of course I could have a missed a bug report about someone with a > huge symlink target. > > Checking what Qt does... ah, PATH_MAX if defined, otherwise a loop which > reallocs twice more every time (starting from 256 bytes). > Personally I'm tempted to just make it 4096, "it ought to be enough for > everybody" as the saying goes :-) > > Xuetian Weng wrote: > So.. is it possible to directly use Qt API there? Why bother handle it > ourselves :) ? I explain that in the comments in the patch: because Qt resolves relative symlink targets to absolute ones, which is exactly what triggered this bug in the first place. I think it makes sense for Qt to do so; the 99% use case for symlinks is to read the target file (or dir). In our case however, to copy a symlink, we do care whether it's relative or absolute. - David --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/125515/#review86340 --- On Oct. 4, 2015, 9:24 a.m., David Faure wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/125515/ > --- > > (Updated Oct. 4, 2015, 9:24 a.m.) > > > Review request for KDE Frameworks. > > > Bugs: 352927 > https://bugs.kde.org/show_bug.cgi?id=352927 > > > Repository: kio > > > Description > --- > > BUG: 352927 > Change-Id: I7d3c988da32cae9d14750c8adb9ca5af6d140572 > > > Diffs > - > > autotests/jobtest.h ef8c3e111ec647cc59c5a9715ab3220ce1651c9e > src/ioslaves/file/file.cpp a0a533c957628b00eff175a949685d4497c5f095 > autotests/jobtest.cpp c24bcead70f78f2bec3b938819fb2fa842e937d5 > > Diff: https://git.reviewboard.kde.org/r/125515/diff/ > > > Testing > --- > > 2 unit tests added > > > Thanks, > > David Faure > > ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 125515: Preserve relative link targets when copying symlinks.
> On Oct. 4, 2015, 5:45 p.m., Frank Reininghaus wrote: > > Looks good! The only thing that I'm not sure about is whether 1000 bytes > > are guaranteed to be enough. Some quick Googling tells me that path lengths > > of 4096 are possible. Maybe we could allocate a temporary large array on > > the heap if the readlink call fails with the 1000 byte buffer on the stack? > > Xuetian Weng wrote: > Just FYI, even PATH_MAX may not exists on some system (AFAIK, hurd, > allowed by posix). > And though st.st_size is mentioned in readlink man page, it may not > return useful value. > > > More information > http://stackoverflow.com/questions/9385386/howto-use-readlink-with-dynamic-memory-allocation > > David Faure wrote: > Just for info, I basically restored this code from kdelibs4 :) > But of course I could have a missed a bug report about someone with a > huge symlink target. > > Checking what Qt does... ah, PATH_MAX if defined, otherwise a loop which > reallocs twice more every time (starting from 256 bytes). > Personally I'm tempted to just make it 4096, "it ought to be enough for > everybody" as the saying goes :-) So.. is it possible to directly use Qt API there? Why bother handle it ourselves :) ? - Xuetian --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/125515/#review86340 --- On Oct. 4, 2015, 9:24 a.m., David Faure wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/125515/ > --- > > (Updated Oct. 4, 2015, 9:24 a.m.) > > > Review request for KDE Frameworks. > > > Bugs: 352927 > https://bugs.kde.org/show_bug.cgi?id=352927 > > > Repository: kio > > > Description > --- > > BUG: 352927 > Change-Id: I7d3c988da32cae9d14750c8adb9ca5af6d140572 > > > Diffs > - > > autotests/jobtest.h ef8c3e111ec647cc59c5a9715ab3220ce1651c9e > src/ioslaves/file/file.cpp a0a533c957628b00eff175a949685d4497c5f095 > autotests/jobtest.cpp c24bcead70f78f2bec3b938819fb2fa842e937d5 > > Diff: https://git.reviewboard.kde.org/r/125515/diff/ > > > Testing > --- > > 2 unit tests added > > > Thanks, > > David Faure > > ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 125515: Preserve relative link targets when copying symlinks.
> On Oct. 4, 2015, 5:45 p.m., Frank Reininghaus wrote: > > Looks good! The only thing that I'm not sure about is whether 1000 bytes > > are guaranteed to be enough. Some quick Googling tells me that path lengths > > of 4096 are possible. Maybe we could allocate a temporary large array on > > the heap if the readlink call fails with the 1000 byte buffer on the stack? > > Xuetian Weng wrote: > Just FYI, even PATH_MAX may not exists on some system (AFAIK, hurd, > allowed by posix). > And though st.st_size is mentioned in readlink man page, it may not > return useful value. > > > More information > http://stackoverflow.com/questions/9385386/howto-use-readlink-with-dynamic-memory-allocation Just for info, I basically restored this code from kdelibs4 :) But of course I could have a missed a bug report about someone with a huge symlink target. Checking what Qt does... ah, PATH_MAX if defined, otherwise a loop which reallocs twice more every time (starting from 256 bytes). Personally I'm tempted to just make it 4096, "it ought to be enough for everybody" as the saying goes :-) - David --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/125515/#review86340 --- On Oct. 4, 2015, 9:24 a.m., David Faure wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/125515/ > --- > > (Updated Oct. 4, 2015, 9:24 a.m.) > > > Review request for KDE Frameworks. > > > Bugs: 352927 > https://bugs.kde.org/show_bug.cgi?id=352927 > > > Repository: kio > > > Description > --- > > BUG: 352927 > Change-Id: I7d3c988da32cae9d14750c8adb9ca5af6d140572 > > > Diffs > - > > autotests/jobtest.h ef8c3e111ec647cc59c5a9715ab3220ce1651c9e > src/ioslaves/file/file.cpp a0a533c957628b00eff175a949685d4497c5f095 > autotests/jobtest.cpp c24bcead70f78f2bec3b938819fb2fa842e937d5 > > Diff: https://git.reviewboard.kde.org/r/125515/diff/ > > > Testing > --- > > 2 unit tests added > > > Thanks, > > David Faure > > ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 125515: Preserve relative link targets when copying symlinks.
> On Oct. 4, 2015, 5:45 p.m., Frank Reininghaus wrote: > > Looks good! The only thing that I'm not sure about is whether 1000 bytes > > are guaranteed to be enough. Some quick Googling tells me that path lengths > > of 4096 are possible. Maybe we could allocate a temporary large array on > > the heap if the readlink call fails with the 1000 byte buffer on the stack? Just FYI, even PATH_MAX may not exists on some system (AFAIK, hurd, allowed by posix). And though st.st_size is mentioned in readlink man page, it may not return useful value. More information http://stackoverflow.com/questions/9385386/howto-use-readlink-with-dynamic-memory-allocation - Xuetian --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/125515/#review86340 --- On Oct. 4, 2015, 9:24 a.m., David Faure wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/125515/ > --- > > (Updated Oct. 4, 2015, 9:24 a.m.) > > > Review request for KDE Frameworks. > > > Bugs: 352927 > https://bugs.kde.org/show_bug.cgi?id=352927 > > > Repository: kio > > > Description > --- > > BUG: 352927 > Change-Id: I7d3c988da32cae9d14750c8adb9ca5af6d140572 > > > Diffs > - > > autotests/jobtest.h ef8c3e111ec647cc59c5a9715ab3220ce1651c9e > src/ioslaves/file/file.cpp a0a533c957628b00eff175a949685d4497c5f095 > autotests/jobtest.cpp c24bcead70f78f2bec3b938819fb2fa842e937d5 > > Diff: https://git.reviewboard.kde.org/r/125515/diff/ > > > Testing > --- > > 2 unit tests added > > > Thanks, > > David Faure > > ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 125515: Preserve relative link targets when copying symlinks.
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/125515/#review86340 --- Looks good! The only thing that I'm not sure about is whether 1000 bytes are guaranteed to be enough. Some quick Googling tells me that path lengths of 4096 are possible. Maybe we could allocate a temporary large array on the heap if the readlink call fails with the 1000 byte buffer on the stack? - Frank Reininghaus On Okt. 4, 2015, 9:24 vorm., David Faure wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/125515/ > --- > > (Updated Okt. 4, 2015, 9:24 vorm.) > > > Review request for KDE Frameworks. > > > Bugs: 352927 > https://bugs.kde.org/show_bug.cgi?id=352927 > > > Repository: kio > > > Description > --- > > BUG: 352927 > Change-Id: I7d3c988da32cae9d14750c8adb9ca5af6d140572 > > > Diffs > - > > autotests/jobtest.h ef8c3e111ec647cc59c5a9715ab3220ce1651c9e > src/ioslaves/file/file.cpp a0a533c957628b00eff175a949685d4497c5f095 > autotests/jobtest.cpp c24bcead70f78f2bec3b938819fb2fa842e937d5 > > Diff: https://git.reviewboard.kde.org/r/125515/diff/ > > > Testing > --- > > 2 unit tests added > > > Thanks, > > David Faure > > ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Review Request 125515: Preserve relative link targets when copying symlinks.
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/125515/ --- Review request for KDE Frameworks. Bugs: 352927 https://bugs.kde.org/show_bug.cgi?id=352927 Repository: kio Description --- BUG: 352927 Change-Id: I7d3c988da32cae9d14750c8adb9ca5af6d140572 Diffs - autotests/jobtest.h ef8c3e111ec647cc59c5a9715ab3220ce1651c9e src/ioslaves/file/file.cpp a0a533c957628b00eff175a949685d4497c5f095 autotests/jobtest.cpp c24bcead70f78f2bec3b938819fb2fa842e937d5 Diff: https://git.reviewboard.kde.org/r/125515/diff/ Testing --- 2 unit tests added Thanks, David Faure ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
