Re: [PATCH] fix imap_mail actually use the rpath option

Hi, few comments just from reading the patch, not the full context, not testing it: On Thu, 2011-08-04 at 16:30 +0100, Keloran wrote: > Index: ext/imap/php_imap.c > =================================================================== > --- ext/imap/php_imap.c (revision 314217) > +++ ext/imap/php_imap.c (working copy) > @@ -4016,7 +4016,27 @@ > if (!INI_STR("sendmail_path")) { > return 0; > } > - sendmail = popen(INI_STR("sendmail_path"), "w"); > + > + /** Used to make return-path work **/ > + char *sendmail_path = INI_STR("sendmail_path"); > + char *appended_sendmail_path = NULL; This won't work on all compiles, declarations have to go first in the block. > + /** Return Path or not **/ > + if (rpath && rpath[0]) { > + appended_sendmail_path = emalloc( > + strlen(sendmail_path) + 3 + strlen(rpath) + 1); > + strcpy(appended_sendmail_path, sendmail_path); > + strcat(appended_sendmail_path, " -f"); > + strcat(appended_sendmail_path, rpath); > + sendmail_path = appended_sendmail_path; > + } > + > + /** open the sendmail pointer **/ > + sendmail = popen(sendmail_path, "w"); /* New Code */ How do you open a pointer ;-) I think everybody reading the code should know popen() so the first comment isn't really useful; I assume the second comment there was for your purpose. > + if (appended_sendmail_path) > + efree(appended_sendmail_path); > + Always use { } even for a single line (see CODING_STANDARDS) While editing this functions: Should this function not also respect the new mail.force_extra_parameters and mail.log settings, too? (Might need an extra bug report to be reported correctly ...) johannes