RFR-8008118

[Martin Buchholz]

8008118
does not appear to be public - could we fix that?

Here is my alternative set of perfectionist changes:

# Cleanup declaration of "environ"
http://cr.openjdk.java.net/~martin/webrevs/openjdk8/environ

# Fix PATH handling
http://cr.openjdk.java.net/~martin/webrevs/openjdk8/pathv/

that could be rolled into one.

I adopted Christos' suggestion of using
continue;

This version only does two allocations.

Martin


On Mon, Mar 25, 2013 at 2:30 PM, Martin Buchholz <martinrb at google.com>wrote:

> Since we're all on this perfectionist quest for quality here, I noticed
> that we could replace allocation for path components with a single
> allocation using NUL as a separator.  I think I'll go code that up.
>
> Also note to all: if java VMs are created and destroyed without the
> process terminating, there is a small leak here (and in many other places
> in the JDK).  There would be a huge amount of work if we ever wanted to get
> that right (especially if we supported concurrently active JVMs).
>
>
> On Fri, Mar 22, 2013 at 7:38 AM, John Zavgren <john.zavgren at oracle.com>wrote:
>
>> Greetings:
>>
>> I made modifications as per Martin's suggestions:
>> 1.) free pathv on "abort".
>> 2.) allocate memory for storing the "cwd" string, and then copy it into
>> the memory location (to make sure that the contents of the pathv[] array
>> don't refer to memory that's from the stack of the procedure that created
>> it.)
>>
>> Thanks!
>> John
>>
>>
>> http://cr.openjdk.java.net/~jzavgren/8008118/webrev.06/
>>
>> ----- Original Message -----
>> From: martinrb at google.com
>> To: christos at zoulas.com
>> Cc: john.zavgren at oracle.com, core-libs-dev at openjdk.java.net
>> Sent: Friday, March 22, 2013 10:19:24 AM GMT -05:00 US/Canada Eastern
>> Subject: Re: RFR-8008118
>>
>>
>>
>>
>> On Thu, Mar 21, 2013 at 12:11 PM, Christos Zoulas <christos at zoulas.com>wrote:
>>
>>> On Mar 21, 11:36am, john.zavgren at oracle.com (John Zavgren) wrote:
>>> -- Subject: Re: RFR-8008118
>>>
>>> | All:
>>> |
>>> | How does this look?
>>> | 1.) I reverted the for statement formatting change.
>>>
>>> Not exactly. Now it is indented incorrectly.
>>>
>>>
>> Agreed.  Really revert.
>>
>>
>>> | 2.) I removed the goto statement and "inlined" some code instead.
>>>
>>> I prefer to write simpler code that works with both signed and unsigned
>>> indexes:
>>>
>>> +                while (i > 0)
>>> +                    if (pathv[--i] != cwd)
>>> +                        free(pathv[i]);
>>> +
>>>
>>>
>> Christos' suggestion looks pretty good.
>>
>> As Mark noted, need to add missing free of pathv itself.
>>
>>
>>> | 3.) I checked to make sure that we're not freeing memory that we
>>> didn't act=
>>> | ually allocate. (Path vector elements that are empty.)
>>>
>>> You are still using the "./" string literal constant in the code so
>>> you'll
>>> end up freeing it (the compiler will prolly merge the two instances and
>>> you'll get lucky but it is semantically incorrect).
>>>
>>
>> Agreed,
>>
>> pathv[i] = cwd;
>>
>
>