Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:41690
Mailing-List: contact internals-help@lists.php.net; run by ezmlm
Received-SPF: unknown (pb1.pair.com: domain php.net does not designate 192.18.6.24 as permitted sender)
Date: Wed, 05 Nov 2008 19:12:06 +0100
In-reply-to: <4911CA1D.7030008@lemonde.fr>
Sender: Johannes.Schlueter@Sun.COM
To: Olivier Grange-Labat <grange@lemonde.fr>
Cc: internals@lists.php.net
Message-ID: <1225908726.4251.23.camel@goldfinger.johannes.nop>
MIME-version: 1.0
Content-type: text/plain
Content-transfer-encoding: 7BIT
References: <4911CA1D.7030008@lemonde.fr>
Subject: Re: [PHP-DEV] [PATCH] parse_ini_string()
From: johannes@php.net (Johannes =?ISO-8859-1?Q?Schl=FCter?=)

Hi,

On Wed, 2008-11-05 at 17:30 +0100, Olivier Grange-Labat wrote:
> Hello,
> 
> Here's a patch again PHP_5_3 to add a parse_ini_string() function.

In general I think it's good to add that function, a few comments below.
>  
> +static
> +ZEND_BEGIN_ARG_INFO_EX(arginfo_parse_ini_string, 0, 0, 1)
> +    ZEND_ARG_INFO(0, str)

If you come up with something might a bit more verbose (same below in
the prototype)

[..]

> +/* {{{ proto array parse_ini_string(string str [, bool
> process_sections [, int scanner_mode]])
> +   Parse configuration string */
> +PHP_FUNCTION(parse_ini_string)
> +{
> +    char *string = NULL, *str = NULL;
> +    int str_len = 0;

please use tabs for indention, not spaces

> +    zend_bool process_sections = 0;
> +    long scanner_mode = ZEND_INI_SCANNER_NORMAL;
> +    zend_ini_parser_cb_t ini_parser_cb;
> +
> +    if (zend_parse_parameters(ZEND_NUM_ARGS() TSRMLS_CC, "s|bl",
> &str, &str_len, &process_sections, &scanner_mode) == FAILURE) {
> +        RETURN_FALSE;
> +    }
> +
> +    /* Set callback function */
> +    if (process_sections) {
> +        BG(active_ini_file_section) = NULL;
> +        ini_parser_cb = (zend_ini_parser_cb_t)
> php_ini_parser_cb_with_sections;
> +    } else {
> +        ini_parser_cb = (zend_ini_parser_cb_t)
> php_simple_ini_parser_cb;
> +    }
> +
> +    /* Setup string */
> +    string = (char *) emalloc(str_len + 1);
> +    strcpy(string, str);
> +    *(string + str_len + 1) = '\0';

Is that copy really needed? Where is the copy free'd?
If doing the copy please use strlcpy instead of strcpy. About strlcpy
see[1].

johannes

[1] http://www.gratisoft.us/todd/papers/strlcpy.html