Code review comment for lp:~mterry/unity8/cancel-pam-harder

Revision history for this message
Michael Terry (mterry) wrote :

> You can change the two futureWatcher.future().something()
> to futureWatcher.something()

Done.

> About QCoreApplication::processEvents it's a bit on the nasty
> side, is it possible to put the code in a slot attached to
> the futurewatcher finished signal?

This would mean holding on to a map of QFuture objects to pam_handle objects so we'd know which ones need to have pam_end called on them when the thread is done. And keeping a set of waiting-QFuture-responses for each thread.

I just figured threading code and PAM code are each complicated enough that I didn't want to try to be fancy. So I favored the direct route.

But I agree that a manual processEvents isn't super sexy. I can change it to the above if you'd like.

> Also does respond(QString()) need to happen
> repeteadly inside the loop or just once?

PAM can offer multiple prompts in a row. The child-thread will block on each response one at a time in a small loop. QFuture.cancel() will not interrupt a waiting child-thread.

But if you can promise that it *will* interrupt the thread the second it stops waiting, before it can get to waiting on a second prompt, then we only need respond() once. And we could even get rid of the loop and just call processEvents once.

I was not so confident in how cancel() worked. But I haven't dealt with Qt threads often.

« Back to merge proposal