Am 21.05.2012 19:56, schrieb Blue Swirl:
> On Mon, May 21, 2012 at 10:25 AM, Jim Meyering <j...@meyering.net> wrote:
>> Blue Swirl wrote:
>>> On Tue, May 15, 2012 at 1:04 PM, <j...@meyering.net> wrote:
>>>> From: Jim Meyering <meyer...@redhat.com>
>>>>
>>>> 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 <meyer...@redhat.com>
>>>> ---
>>>> 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
>
> 1) (or 3). Though for v1.1, maybe 3) is the smaller fix and later do 1) for
> 1.2.
A patch replacing tabs by spaces isn't really the kind of patches that
we would want to avoid during freeze. It's easy enough to check with git
diff -w that it doesn't change anything semantically.
Kevin
