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

[Thomas Stüfe]

Hi Robin,

Had a short look. Note that I cannot open the issue. Link is wrong, and the
link in the source.patch is wrong too. So, I just looked at the webrev.

Remarks:

-> This is a matter of taste but I prefer pointers to references for output
variables. Makes the intent clearer at the calling site, and that way you
could get rid of the /* output */ comment.

-> in available_memory_container():
Could we also add "using host value" to the logging in error case as you do
in physical_memory_container()?

-> (Not part of your patch) Can OSContainer::memory_limit_in_bytes()
actually ever return "OSCONTAINER_ERROR"? I may be wrong here but:

OSCONTAINER_ERROR = -2. /memory.limit_in_bytes gets returned as julong, so
we have (julong)(-2). That gets compared with julong _unlimited_memory
which is basically LONG_MAX, so signed long max, which should be smaller
than (julong)(-2), or? So OSContainer::memory_limit_in_bytes() should
always return -1 for both errors and the unlimited case.

-> Can we:
-  st->print("container_type: %s\n", p != NULL ? p : "failed");
+  st->print_cr("container_type: %s", p != NULL ? p : "failed");
?

-> char * OSContainer::container_type() - you free() that value but it is
not strdup()ed.

(Btw I do not like that some functions return strduped values, some do not.
If OSContainer::container_type() wants to return a static string, its
return type should be at least const char*, not char*. They even added an
explicit cast to nonconst char* .)

Kind Regards, Thomas



On Wed, Feb 14, 2018 at 2:18 PM, Robbin Ehn <robbin.ehn at oracle.com> wrote:

> 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
>>>>>>
>>>>>