Re: kdereview: ksmtp
Hi Rolf, thanks for the review, sorry it took me so long to get to you. On Tuesday, May 16, 2017 8:05:17 PM CEST Rolf Eike Beer wrote: > Am Donnerstag, 11. Mai 2017, 17:03:01 schrieb Daniel Vrátil: > > Hi, > > > > please review ksmtp, which is now in kdereview. > > -the CMakeLists.txt has a mix of spaces inside () or not Fixed > > -in loginjob, line 173, you check for code 25. Should this be 250? Or is > that 25*? Where is ServerResponse actually defined, I only see the header. ServerResponse is defined in sessionthread.cpp for some reason. ServerResponse::isCode() indeed checks the prefix of the response, so isCode(25) returns true for any 25x return code. > -does that support pipelining? I don't see any sync points, so I guess not. Not yet, but it's next on the todo list. > -there is a longstanding bug in KMail that it violates the RfC when it has a > problem with authentication (e.g. password rejected), that is does not > properly QUIT the SMTP session, but just closes the socket. Is that > properly handled? It is properly handled now. Calling Session::quit() does not close the connection but sends QUIT command and only closes the connection after a response arrives from the server (unless the server closes it first of course). It's now up to the user to ensure that the Session object is not destroyed until the state changes to Disconnected after calling Session::quit(). Dan > > Greetings, > > Eike -- Daniel Vrátil www.dvratil.cz | dvra...@kde.org IRC: dvratil on Freenode (#kde, #kontact, #akonadi, #fedora-kde) GPG Key: 0x4D69557AECB13683 Fingerprint: 0ABD FA55 A4E6 BEA9 9A83 EA97 4D69 557A ECB1 3683 signature.asc Description: This is a digitally signed message part.
Re: kdereview: ksmtp
Am Donnerstag, 11. Mai 2017, 17:03:01 schrieb Daniel Vrátil: > Hi, > > please review ksmtp, which is now in kdereview. -the CMakeLists.txt has a mix of spaces inside () or not -in loginjob, line 173, you check for code 25. Should this be 250? Or is that 25*? Where is ServerResponse actually defined, I only see the header. -does that support pipelining? I don't see any sync points, so I guess not. -there is a longstanding bug in KMail that it violates the RfC when it has a problem with authentication (e.g. password rejected), that is does not properly QUIT the SMTP session, but just closes the socket. Is that properly handled? Greetings, Eike signature.asc Description: This is a digitally signed message part.
Re: kdereview: ksmtp
On Thursday, May 11, 2017 11:11:11 PM CEST Albert Astals Cid wrote: > El dijous, 11 de maig de 2017, a les 17:03:01 CEST, Daniel Vrátil va escriure: > > Hi, > > > > please review ksmtp, which is now in kdereview. > > Are tests supposed to pass? > > 2: QFATAL : SmtpTest::testLoginJob(Plain auth ok) Received signal 11 Hmm, it passes here. Must be some timing issue. I'll see if I can reproduce it on my other system. Dan > > Cheers, > Albert > > > KSMTP is a small simple library with a KJob-based API similar to KIMAP or > > KDAV to send mime messages via SMTP. It was initially written in 2011 by > > Gregory Schlomoff but since then it's been lying around in playground > > without any interest or use. I have recently picked it up, ported to > > Frameworks and improved the job handling and authentication to make the > > library ussable for KDE PIM and would like to have it released as part of > > KDE Applications 17.08. > > > > KDE PIM currently uses a custom KIO Slave to send messages via SMTP, which > > is hard to maintain and extend. Having a Job-based library like KSmtp will > > make it much easier for us to introduce support for example for Google > > XOAUTH2 authentication mechanism. > > > > Thanks, > > Dan -- Daniel Vrátil www.dvratil.cz | dvra...@kde.org IRC: dvratil on Freenode (#kde, #kontact, #akonadi, #fedora-kde) signature.asc Description: This is a digitally signed message part.
Re: kdereview: ksmtp
El dijous, 11 de maig de 2017, a les 17:03:01 CEST, Daniel Vrátil va escriure: > Hi, > > please review ksmtp, which is now in kdereview. Are tests supposed to pass? 2: QFATAL : SmtpTest::testLoginJob(Plain auth ok) Received signal 11 Cheers, Albert > > KSMTP is a small simple library with a KJob-based API similar to KIMAP or > KDAV to send mime messages via SMTP. It was initially written in 2011 by > Gregory Schlomoff but since then it's been lying around in playground > without any interest or use. I have recently picked it up, ported to > Frameworks and improved the job handling and authentication to make the > library ussable for KDE PIM and would like to have it released as part of > KDE Applications 17.08. > > KDE PIM currently uses a custom KIO Slave to send messages via SMTP, which > is hard to maintain and extend. Having a Job-based library like KSmtp will > make it much easier for us to introduce support for example for Google > XOAUTH2 authentication mechanism. > > Thanks, > Dan
Re: kdereview: ksmtp
Thanks, totally forgot to run clazy/krazy on the codebase. On Thursday, May 11, 2017 10:25:36 PM CEST Allen Winter wrote: > ran clazy level2 . no hits > > ran krazy checks and it found: > . Check for C++ ctors that should be declared 'explicit' [explicit]... 1 > issue found ./autotests/fakeserver.h: line#36 (1) Fixed > > . Check for normalized SIGNAL and SLOT signatures [normalize]... 4 issues > found ./src/session.cpp: SIGNALS: line#285,286 (2) > ./src/session.cpp: SLOTS: line#285,286 (2) Fixed (converted to the new connect() syntax) > . Check for strings used improperly or should be i18n. [strings]... 5 issues > found ./autotests/fakeserver.cpp: QLatin1String issues > line#172,175,190,201,218 (5) Those are false positives. I guess krazy assumes .startsWith() to be QString::startsWith(), while those are QByteArray::startsWith(). Dan > On Thursday, May 11, 2017 11:03:01 AM EDT Daniel Vrátil wrote: > > Hi, > > > > please review ksmtp, which is now in kdereview. > > > > KSMTP is a small simple library with a KJob-based API similar to KIMAP or > > KDAV to send mime messages via SMTP. It was initially written in 2011 by > > Gregory Schlomoff but since then it's been lying around in playground > > without any interest or use. I have recently picked it up, ported to > > Frameworks and improved the job handling and authentication to make the > > library ussable for KDE PIM and would like to have it released as part of > > KDE Applications 17.08. > > > > KDE PIM currently uses a custom KIO Slave to send messages via SMTP, which > > is hard to maintain and extend. Having a Job-based library like KSmtp > > will make it much easier for us to introduce support for example for > > Google XOAUTH2 authentication mechanism. > > > > Thanks, > > Dan -- Daniel Vrátil www.dvratil.cz | dvra...@kde.org IRC: dvratil on Freenode (#kde, #kontact, #akonadi, #fedora-kde) GPG Key: 0x4D69557AECB13683 Fingerprint: 0ABD FA55 A4E6 BEA9 9A83 EA97 4D69 557A ECB1 3683 signature.asc Description: This is a digitally signed message part.
kdereview: ksmtp
Hi, please review ksmtp, which is now in kdereview. KSMTP is a small simple library with a KJob-based API similar to KIMAP or KDAV to send mime messages via SMTP. It was initially written in 2011 by Gregory Schlomoff but since then it's been lying around in playground without any interest or use. I have recently picked it up, ported to Frameworks and improved the job handling and authentication to make the library ussable for KDE PIM and would like to have it released as part of KDE Applications 17.08. KDE PIM currently uses a custom KIO Slave to send messages via SMTP, which is hard to maintain and extend. Having a Job-based library like KSmtp will make it much easier for us to introduce support for example for Google XOAUTH2 authentication mechanism. Thanks, Dan -- Daniel Vrátil www.dvratil.cz | dvra...@kde.org IRC: dvratil on Freenode (#kde, #kontact, #akonadi, #fedora-kde) signature.asc Description: This is a digitally signed message part.
