Re: Some more setenv(3) update

[Matthias Scheler <tron%zhadum.org.uk@localhost>]

On Tue, Oct 12, 2010 at 12:06:45PM +0200, Nicolas Joly wrote:
> >        for (cc = name; *cc && *cc != '='; ++cc)        /* no `=' in name */
> >                continue;
> > 
> > before the initial test, and then check for *cc instead of scanning the
> > name twice, once in strchr and a second time in strlen.

It will be cached the second time anyway.

> Right, but we then need to handle case for NULL name.

Which is exactly why your solution was better as it was simpler.

>> +    for (cc = name; cc && *cc && *cc != '='; ++cc)
> +             continue;

It don't like this even a bit. But if we want to keep it it should look
like this:

        for (cc = name; cc != NULL && *cc != '\0' && *cc != '='; ++cc)
                continue;

The style guide says that you shouldn't treat pointers and the likes
as boolean values.

> +
> +     if (name == NULL || value == NULL || *name == '\0' || *cc == '=') {
> +             errno = EINVAL;
> +             return -1;
> +     }
> +

"cc" might be a NULL pointer and the whole function could crash if somebody
 changes the evaluation order.

I would prefer this:

        if (name == NULL || value == NULL) {
                errno = EINVAL;
                return -1;
        }

        size = strcspn(name, "=");
        if (size == 0 || name[size] != '\0') {
                errno = EINVAL;
                return -1;
        }

It only reads the string once and figures out the length and whether
there is an equal sign in the name at the same time.

        Kind regards

-- 
Matthias Scheler                                  http://zhadum.org.uk/