RFR(M): 8038201: Clean up misleading usage of malloc() in init_system_properties_values()

[Lindenmaier, Goetz]

Thanks, Coleen!

Best regards,
  Goetz

-----Original Message-----
From: Coleen Phillimore [mailto:coleen.phillimore at oracle.com] 
Sent: Tuesday, March 25, 2014 3:20 PM
To: Lindenmaier, Goetz; hotspot-dev at openjdk.java.net
Subject: Re: RFR(M): 8038201: Clean up misleading usage of malloc() in init_system_properties_values()


On 3/25/14 10:15 AM, Lindenmaier, Goetz wrote:
> Hi Coleen,
>
> I also thought about moving that to posix.
> But the paths assembled differ a lot on the platforms,
> so I don't think it really helps moving this function:
>   - Bsd has two variants, distinguished for __APPLE__.
>   - AIX calls LD_LIBRARY_PATH   LIB_PATH and only has one extensions dir, and more simple library path.
>   - Solaris has an all different handling of the library path.
> Only the first part is similar, except for __APPLE__.  I don't think moving
> this part makes the code better maintainable, as it would spread related
> code.

Okay, I saw your reply to Vladimir after I made this comment.  They just 
looked the same so it's always a question.   Too bad.

I didn't actually look at the contents of the changes.  If you need 
another reviewer, I will, but I think you have good reviewers in 
Vladimir and Dmitry.

Thanks,
Coleen

>
> Best regards,
>    Goetz.
>
> -----Original Message-----
> From: hotspot-dev [mailto:hotspot-dev-bounces at openjdk.java.net] On Behalf Of Coleen Phillimore
> Sent: Dienstag, 25. März 2014 14:46
> To: hotspot-dev at openjdk.java.net
> Subject: Re: RFR(M): 8038201: Clean up misleading usage of malloc() in init_system_properties_values()
>
>
> If os::init_system_properties() is really the same for all of these
> platforms, I would like to see it moved to os_posix.cpp.   It doesn't
> seem to make sense to change it the same way 4 times.  Jerry Thornbrough
> is working on the RFE to move "posix" into "xnix", and make more code
> common but this could precede that.
>
> Thanks,
> Coleen
>
> On 3/25/14 1:13 AM, Vladimir Kozlov wrote:
>> Hi Goetz,
>>
>> Do you really have to change /**/ comments to //?
>> Without that you would get much clear visible changes.
>>
>> Changes looks fine to me. I wish we could do this in os_posix.cpp but
>> it is for an other time.
>>
>> Thanks,
>> Vladimir
>>
>> On 3/24/14 2:50 PM, Lindenmaier, Goetz wrote:
>>> Hi,
>>>
>>> please review and test this change. I please need a sponsor.
>>> http://cr.openjdk.java.net/~goetz/webrevs/8038201-sec/webrev.00/
>>>
>>> This change addresses the implementation of
>>> init_system_properties_values
>>> on aix, linux, solaris and bsd.
>>> In init_system_properties_values a macro was defined mapping malloc to
>>> NEW_C_HEAP_ARRAY. NEW_C_HEAP_ARRAY checks for successful allocation
>>> or exits the VM.  The code of this method handles the allocated
>>> pointers as
>>> allocated with real malloc, i.e.,  there were checks for NULL and
>>> calls to free().
>>>
>>> This change replaces the macro malloc with NEW_C_HEAP_ARRAY and removes
>>> the unnecessary checks making the code clearer.  Also, it uses a
>>> local array where
>>> possible.
>>>
>>> The allocated memory is passed to calls that end up at
>>> SystemProperty::set_value().
>>> set_value copies the strings passed.  Thus the memory allocated in
>>> init_system_properties_values must be freed after these calls. Most
>>> of these
>>> frees were missing.
>>>
>>> This change adds the missing frees.
>>>
>>> Testing this change I ran into a warning in ppc code which I fixed, too.
>>> I did some local test.  I'll get broader tests by Wednesday.
>>>
>>> Best regards,
>>>     Goetz.
>>>
>>>