D6709: [RFC] Add support for sharing file descriptor between KIO slave and KAuth helper

[David Faure]

dfaure requested changes to this revision.
dfaure added inline comments.
This revision now requires changes to proceed.

INLINE COMMENTS

> sharefd.cpp:32
> +
> +//borrowed from klocalsocket.cpp
> +class KSockaddrUn

Either move it to a common header (if QString everywhere is OK), or use a different classname here, to avoid a classname clash at runtime (and document that it was ported to std::string, so we know why it was forked)

> sharefd.cpp:67
> +
> +class KMsgHdr
> +{

Can you make the classname more descriptive? ShareFDMessageHeader maybe?
>From the looks of it I initially thought this came from kio and not just from sharefd.*

> sharefd.cpp:95
> +
> +// File descriptor reciever
> +FdReceiver::FdReceiver(const QString &path, QObject *parent)

typo: receiver

> sharefd.cpp:147
> +{
> +    if (m_readNotifier) {
> +        delete m_readNotifier;

this if() serves no purpose (delete nullptr is a well-defined no-op)

> sharefd.cpp:159
> +
> +// File descriptor sender
> +FdSender::FdSender(const std::string &path)

If FDSender is only used in the kauthhelper, and FDReceiver is only used in kio_file, why not split up this .cpp into two, and only compile the bit that's needed by each target?
(you'll need a private header for the shared classes of course, i.e. the sockaddr wrapper and the messageheader would go into sharefd_p.h)

It would also make the QString vs std::string API difference look less weird.

And then the files can match the classnames, too (fdsender.cpp/h and fdreceiver.cpp/h)

> sharefd.cpp:166
> +    if (m_socketDes == -1) {
> +        std::cerr << "socket error:" << strerror(errno);
> +        return;

you need std::endl when using std::cerr  (repeats)

REVISION DETAIL
  https://phabricator.kde.org/D6709

To: chinmoyr, thiago, #frameworks, dfaure
Cc: dfaure, davidedmundson, elvisangelaccio, shortstheory
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20170903/b7547221/attachment-0001.html>