RFR: 8197408: Bad pointer comparison and small cleanup in os_linux.cpp

[Robbin Ehn]

Ping!

/Robbin

On 2018-02-08 13:35, Robbin Ehn wrote:
> Hi David,
> 
> On 02/08/2018 01:19 PM, David Holmes wrote:
>> On 8/02/2018 10:08 PM, Robbin Ehn wrote:
>>> Hi David,
>>>
>>> On 02/08/2018 12:43 PM, David Holmes wrote:
>>>> Hi Robbin,
>>>>
>>>> On 8/02/2018 7:03 PM, Robbin Ehn wrote:
>>>>> Hi all,
>>>>>
>>>>> There is a bad pointer comparison in os_linux.cpp while looking at that 
>>>>
>>>> You seem to be missing the fact that OSContainer::cpu_cpuset_memory_nodes() can return a pointer or an error code.
>>>
>>> This is not true for macro:
>>> GET_CONTAINER_INFO_CPTR
>>>
>>> As far I can see?
>>
>> Sorry - you're right. I misread current code and misremembered what happened at the initial code review - where I'm sure this "pointer versus error code" issue was also flagged.
>>
>>> It returns:
>>> if (err != 0)
>>>    return (return_type) NULL;
>>> or:
>>> return os::strdup(mems);
>>>
>>> If you know a method that returns an integer in a char*, it's broken should be fixed.
>>>
>>>>
>>>>> I saw some if statement were missing bracket, a lot of extra scopes and complexity in the scoping.
>>>>
>>>> You'd better check all this with Bob Vandette as its his container support code.
>>>
>>> Not sure what you mean. It passes container tests.
>>
>> In what environment did you run the container tests? Most of the code you've been refactoring deals with various errors and misconfigurations that can occur.
>>
>> I'm sure Bob will want a chance to check the refactoring still does as he intended.
> 
> On mach5 all platforms with the container test (+ hotspot_tier1) and locally.
> I said before, regarding containers, all Linux have cgroups configured so this logic always thinks we are in a container and does this logging and calculation.
> 
> But no there is no tests for miss-configuration that I found.
> 
> Added Bob!
> 
> Thanks, Robbin
> 
> 
>>
>> Cheers,
>> David
>> -----
>>
>>>
>>> Thanks, Robbin
>>>
>>>>
>>>> Cheers,
>>>> David
>>>>
>>>>> Webrev:
>>>>> http://cr.openjdk.java.net/~rehn/8197408/webrev/
>>>>> Bug:
>>>>> http://cr.openjdk.java.net/~rehn/8197408/webrev/
>>>>>
>>>>> Thanks, Robbin