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
