[Dovecot] "pipe" plugin - is anyone interested (or using it)?

[Nicolas Boullis]

Timo Sirainen wrote:
> 
> I downloaded it when you first mentioned it, but looks like I never got
> around to actually looking at it. Yes, something like this would be
> nice..
> 
> One thing at least that should be changed is to configure it using
> virtual mailbox names instead of full mailbox paths. This isn't really
> possible with v1.0, but v1.1 should make it pretty easy.

You already told me about this last time, but I haven't yet come to read
the source code of dovecot 1.1...


> A few things about the code:
> 
> 	save_dest_mail = mail_alloc(ctx->transaction,
> 				    MAIL_FETCH_PHYSICAL_SIZE, NULL);
> 
> Quota plugin wanted to know the message's physical size, but your plugin
> doesn't need it. So you could just use 0 instead of
> MAIL_FETCH_PHYSICAL_SIZE.

Thanks for pointing this.


> I think write() can return partially written data if the other side
> isn't reading it fast enough. Using write_full() instead would anyway be
> better/safer.

I just reread my code, and I think my use of write looks safe, since
only the amount that was correctly written is skipped with
i_stream_skip. Do you think I'm missing something?


> Do you really need to wait for the executed process to finish? Since
> this is the only plugin currently creating child processes, I'd setup a
> SIGCHLD handler and waitpid() there to get rid of the zombie:
> lib_signals_set_handler(SIGCHLD, TRUE, chld_handler, NULL);

Waiting is required if we want the append to fail if the command fails.
I guess it should better be a configurable option, don't you think so?


> Multiple commands are now processed sequentially. I don't know if
> there's real need for multiple commands, but it would be faster to read
> the message input just once and send it to all pipes in parallel.

I don't think there is real need for multiple commands, but I think it
was easier for me to program things like this... ;-)


> "return 0 * WEXITSTATUS(status);" returns always 0 :)

Hummm... I guess I should avoid drinking alcohol before I program... ;-)
It probably should be "return WEXITSTATUS(status);"; I have no idea why
I changed this in such a strange way...


> I'd also leave stderr as-is for the child process so it could log
> errors, and for handling syscall failures use:
> i_fatal("dup2() failed: %m");

OK, point taken.


One question still: would you consider merging my plugin in dovecot if I
ported it to 1.1?


Cheers,

Nicolas