dfaure requested changes to this revision. dfaure added a comment. This revision now requires changes to proceed.
Good work, just some small things, and a suggested simplification to the overall slave<->job protocol. INLINE COMMENTS > slavebase.h:941 > + * with elevated privileges. It is similar to data() but has different > name > + * so as not to mess up existing connections. > + */ It's not similar to data(), it doesn't send data from the file being read ;-) Please remove that sentence from the docu, and rename the signal (see https://phabricator.kde.org/D6832). > slaveinterface.cpp:323 > + case MSG_PRIVILEGE_EXEC: > + emit privilegeExecData(rawdata); > + break; Do the bytearray -> enum conversion here, so that everyone else than slavebase and slaveinterface (i.e. both the job and the slave) only see the enum. QDataStream can be used to stream an int (the enum value) in and out, no need to use "strings" in the bytearray. Alternatively, QByteArray::number() can do the job as long as it's really just the one enum value that's needed here (and we know it will never need to be more...). > slaveinterface.h:89 > // add new ones here once a release is done, to avoid breaking binary > compatibility > + MSG_PRIVILEGE_EXEC > }; Move the new value above the "add new ones here" comment, otherwise the next person will break BC. > file_unix.cpp:670 > + > + privilegeExecData("AskConfirmation"); > + readPrivilegeExecData(jobReply); Wait, why the two step roundtrip here? Instead of "can you become root?" / "yes I can" / "ok please ask the user" / "OK, the user said yes", the protocol here could really be simplified to "request to elevate privilege" "OK (it's enabled, and the user said yes)". Which even removes the need for any enum, it's really just one request and one reply which is true or false. > file_unix.cpp:691 > + QStringList testData; > + testData += execAction.name(); > + testData += QString::number(execAction.isValid()); use C++11 initializer to avoid reallocations, i.e. `QStringList testData{execAction.name(), ..., ..., ...};` Ah but then you just use a join(","), so the most efficient thing to do here would be to drop the QStringList and do const QString metaData = execAction.name() + ',' + QString::number... + ',' + etc. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D6829 To: chinmoyr, dfaure, #frameworks Cc: #frameworks
