dfaure requested changes to this revision. dfaure added inline comments. This revision now requires changes to proceed.
INLINE COMMENTS > chinmoyr wrote in copyjob.cpp:1674 > Ideally it should be set when m_privilegeExecutionEnabled is true. But it > will add more lines. Even though its just 2-3 lines, it doesn't look good. > Besides the flag is ineffective if the parent job doesn't have this flag set. > Shall I remove it? To me it looks worse to unconditionally pass a security-critical flag when it shouldn't be set, it reads like a security hole (even if, as you say, upon further research it turns out that the flag has no effect). Maybe make it a helper method flagsForSubJob() to avoid any duplication. BTW isn't the flag missing for the KIO::symlink case above? > copyjob.cpp:294 > + break; > + default: > + break; Remove default statement, which would leave the variable uninitialized. This way the compiler will warn if a new value is added to the CopyMode enum. REVISION DETAIL https://phabricator.kde.org/D6833 To: chinmoyr, dfaure, #frameworks Cc: #frameworks
