This revision was automatically updated to reflect the committed changes.
Closed by commit R303:bbf120b78773: Delete IdleSlave having temporary
authorization (authored by chinmoyr).
REPOSITORY
R303 KInit
CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D10824?vs=28536&id=31255
REVISION
dfaure accepted this revision.
dfaure added a comment.
This revision is now accepted and ready to land.
OK. If the slave indeed gets killed then this is what we want, indeed.
REPOSITORY
R303 KInit
BRANCH
master
REVISION DETAIL
https://phabricator.kde.org/D10824
To: chinmoyr, dfaure, #
chinmoyr added a comment.
David, what about this patch? It seems to me deleting the IdleSlave object is
the only way to kill the ioslave from here.
REPOSITORY
R303 KInit
REVISION DETAIL
https://phabricator.kde.org/D10824
To: chinmoyr, dfaure, #frameworks
Cc: #frameworks, michaelh, ngrah
chinmoyr added a comment.
I haven't yet tried your patch (which btw involves the http ioslave right?)
but I am quite sure that exit() is not called for file ioslave after
interrupting the application. I have placed the debug statements inside exit()
as well as before every exit() call and I
dfaure added a comment.
I tested what happens in the current code when an idle job is killed. My
testcase was configuring to use KIO in componentchooser ("open http urls in an
application based on the contents on the URL") and then `kioclient5 exec
www.kde.org`. This puts the slave on hold w
chinmoyr added a comment.
Neither SlaveBase::exit nor Slave::kill is called. I also commented the line
in ConnectionBackend which removes the socket file but even then the slave
process terminated.
At this point I have no idea why is this working.
REPOSITORY
R303 KInit
REVISION DETAIL
dfaure added a comment.
Interesting. But are you sure that neither Slave::kill() nor
SlaveBase::exit() is called? It can't just be the deleteLater that does this,
unless I'm missing something (e.g. closing the pipe makes the slave die with
SIGPIPE maybe)
REPOSITORY
R303 KInit
REVISION DE
chinmoyr added a comment.
Actually deleteLater() does kill the slave. I placed a qDebug statement in
there and immediately after the message I am getting
kdeinit5: PID $slave_pid terminated.
REPOSITORY
R303 KInit
REVISION DETAIL
https://phabricator.kde.org/D10824
To: chinmoyr, df
dfaure added a comment.
No, that deletes the IdleSlave QObject, but the slave process is still
running then.
IdleSlave::~IdleSlave()
{
}
Not much happening there ;)
Yes, that makes me wondering if the SLAVE_MAX_IDLE thing actually works.
Hmm, and KLauncher::reques
chinmoyr added inline comments.
INLINE COMMENTS
> dfaure wrote in klauncher.cpp:1114
> This deletes the Slave C++ class, but it doesn't actually kill the ioslave.
> So why do it?
>
> I'm confused now. Do you want to kill the ioslave (then call slave->kill())
> or do you want to reuse that ioslav
dfaure requested changes to this revision.
dfaure added inline comments.
This revision now requires changes to proceed.
INLINE COMMENTS
> klauncher.cpp:1114
> +mSlaveList.removeAll(slave);
> +slave->deleteLater();
> +}
This deletes the Slave C++ class, but it doesn't actually
chinmoyr updated this revision to Diff 28536.
chinmoyr added a comment.
Used deleteLater()
REPOSITORY
R303 KInit
CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D10824?vs=28002&id=28536
BRANCH
master
REVISION DETAIL
https://phabricator.kde.org/D10824
AFFECTED FILES
src/klau
dfaure requested changes to this revision.
dfaure added a comment.
This revision now requires changes to proceed.
Wait, no, this is in a slot connected to the slave that we're deleting ==>
you have to use deleteLater.
REPOSITORY
R303 KInit
REVISION DETAIL
https://phabricator.kde.org/D108
dfaure accepted this revision.
This revision is now accepted and ready to land.
REPOSITORY
R303 KInit
BRANCH
master
REVISION DETAIL
https://phabricator.kde.org/D10824
To: chinmoyr, dfaure, #frameworks
Cc: #frameworks, michaelh
lbeltrame added a reviewer: Frameworks.
REPOSITORY
R303 KInit
REVISION DETAIL
https://phabricator.kde.org/D10824
To: chinmoyr, dfaure, #frameworks
Cc: #frameworks, michaelh
chinmoyr created this revision.
chinmoyr added a reviewer: dfaure.
Restricted Application added a project: Frameworks.
Restricted Application added a subscriber: Frameworks.
chinmoyr requested review of this revision.
REVISION SUMMARY
IdleSlave with temporary authorization can be easily misused.
16 matches
Mail list logo