Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:54370
Mailing-List: contact internals-help@lists.php.net; run by ezmlm
Received-SPF: error (pb1.pair.com: domain schlueters.de from 217.114.211.66 cause and error)
To: Keloran <ava3ar@gmail.com>
Cc: PHP Internals <internals@lists.php.net>
In-Reply-To: <CAF=va1VoO38yrGXpfXk+L+YZ-BiLewBe4QPkvpp7QM7MvEwn+A@mail.gmail.com>
References:
	 <CAF=va1VoO38yrGXpfXk+L+YZ-BiLewBe4QPkvpp7QM7MvEwn+A@mail.gmail.com>
Content-Type: text/plain; charset="UTF-8"
Date: Thu, 04 Aug 2011 18:48:09 +0200
Message-ID: <1312476489.1541.28.camel@guybrush>
Mime-Version: 1.0
Content-Transfer-Encoding: 7bit
Subject: Re: [PHP-DEV] [PATCH] fix imap_mail actually use the rpath option
From: johannes@schlueters.de (Johannes =?ISO-8859-1?Q?Schl=FCter?=)

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