Re: [Qemu-devel] [PATCH 1/3] envlist.c: handle strdup failure

[Jim Meyering]

Blue Swirl wrote:
> On Tue, May 15, 2012 at 1:04 PM,  <address@hidden> wrote:
>> From: Jim Meyering <address@hidden>
>>
>> Without this, envlist_to_environ may silently fail to copy all
>> strings into the destination buffer, and both callers would leak
>> any env strings allocated after a failing strdup, because the
>> freeing code stops at the first NULL pointer.
>>
>> Signed-off-by: Jim Meyering <address@hidden>
>> ---
>>  envlist.c | 9 ++++++++-
>>  1 file changed, 8 insertions(+), 1 deletion(-)
>>
>> diff --git a/envlist.c b/envlist.c
>> index f2303cd..2bbd99c 100644
>> --- a/envlist.c
>> +++ b/envlist.c
>> @@ -235,7 +235,14 @@ envlist_to_environ(const envlist_t *envlist, size_t 
>> *count)
>>
>>        for (entry = envlist->el_entries.lh_first; entry != NULL;
>>            entry = entry->ev_link.le_next) {
>> -               *(penv++) = strdup(entry->ev_var);
>> +               if ((*(penv++) = strdup(entry->ev_var)) == NULL) {
>> +                       char **e = env;
>> +                       while (e != penv) {
>> +                               free(*e++);
>> +                       }
>> +                       free(env);
>> +                       return NULL;
>> +               }
>
> ERROR: code indent should never use tabs
> #82: FILE: envlist.c:238:
> +^I^Iif ((*(penv++) = strdup(entry->ev_var)) == NULL) {$

That entire file is indented solely with TABs, so adding these new
lines using spaces for indentation seems unjustified: the mix tends
to make the code unreadable in some contexts (email quoting, for one).
How about two patches: one to convert all leading TABs in envlist.c to
spaces, and the next to make the above change, but indenting with spaces?

> ERROR: do not use assignment in if condition
> #82: FILE: envlist.c:238:
> +               if ((*(penv++) = strdup(entry->ev_var)) == NULL) {

I agree with the sentiment, but found that the alternative was less
readable and less maintainable, since I'd have to increment "penv" in
two places (both in the if-block and after it) rather than in just one.
However, I've just realized I can hoist the "penv++" increment into the
"for-statement", in which case it's ok:

        for (entry = envlist->el_entries.lh_first; entry != NULL;
             entry = entry->ev_link.le_next, penv++) {
                *penv = strdup(entry->ev_var);
                if (*penv == NULL) {
                        char **e = env;
                        while (e <= penv) {
                                free(*e++);
                        }
                        free(env);
                        return NULL;
                }
        }

Your move.  Which would you prefer?
  1) two patches: one replacing all leading TABs with equivalent spaces,
      then the above patch
  2) one patch, indented using TABs, in spite of the checkpatch failure
  3) one patch, indented using spaces, in spite of the consistency issue

...
> total: 9 errors, 0 warnings, 15 lines checked
>
> Please fix.