hannesm / jackline

minimalistic secure XMPP client in OCaml
BSD 2-Clause "Simplified" License
250 stars 20 forks source link

Better GUI integration for notifications #131

Closed infinity0 closed 8 years ago

infinity0 commented 8 years ago

Add the ability to communicate with the parent process via file descriptors / pipes. This allows us to provide more accurate notifications. Also remove notifications.state as suggested earlier.

There's a few TODO items remaining, which I'll complete if you like the general direction of this PR.

There should be no increase in privacy leaks to the surrounding processes - I didn't change the information that jackline sends out (only added another sending mechanism), and I added the ability to receive a minor amount of information which I believe is necessary to make it work more nicely with GUIs (and allows us to avoid sending out contact ids, as I suggested earlier). I understand this might be contentious though, so I await your input.

The fd-based approach was inspired by dialog(1) and gpg(1). It's a bit easier to code with than using unix sockets. I can also do the latter if desired though.

hannesm commented 8 years ago

this looks interesting.. I'm a bit curious why to have two file descriptors (since they're bidirectional, shouldn't one suffice)?

infinity0 commented 8 years ago

Pipes are unidirectional, and this is the use-case I have in mind - either a direct anonymous pipe to the parent process or to a named pipe created by it.

infinity0 commented 8 years ago

Ping - have updated first commit as you said, then added another commit to address the other thing that I spotted.

hannesm commented 8 years ago

thx... but won't manage to review this tonight... why did you rename so many symbols?

infinity0 commented 8 years ago

sorry, I thought the new names were a bit more descriptive:

I perhaps went a bit overboard with the last two, so could change these back if you prefer.

Apart from renames, I did also move some stuff around, but this is only because ocaml forces you to define things in a topological order, i.e. you can't refer to stuff that's defined later on in the file.

infinity0 commented 8 years ago

Hey, any news on this? Shall I revert some of the renames?

hannesm commented 8 years ago

if you would separate the "Better GUI integration" from the other parts you did in this PR (rename, ...), that'd for sure speed up the process to review, test and merge... (sorry, I'm lazy and do not have the needs of this PR for my setup; but I'm willing to merge if I can convince myself that it doesn't leak fds etc.)

infinity0 commented 8 years ago

Done - you can test with e.g. ./jackline.native --fd-nfy 4 4>~/tmp/jackline.log.

(Github's commit list is badly-ordered because I rewrote some commits, the last one is 96e5952).

hannesm commented 8 years ago

thanks! your proposed test does not work, it feels very brittle to depend on /dev/fd/ -- wouldn't a filename be more appropriate (and if you have your /dev/fd/4, you can use that as a filename)?

hannesm commented 8 years ago

I manually merged most of your commits [not state_mvar -> notify_mvar] into master, and did some modifications (stylistic, also Lwt_unix.openfile instead of Unix.openfile (thus need to run inside of Lwt main), and use a filename instead of a file descriptor number for the params (while FreeBSD can have a /dev/fd file system, it does not so by default)).

infinity0 commented 8 years ago

Thanks! Yes I guess for OCaml currently, using a filename is cleaner. (Originally, int fds was to allow one to avoid mkfifo - then one can create an anonymous pipe which is cleaned up automatically when the parent exits, whereas with mkfifo you need to add clean up code to the parent. It's not so bad, though.)