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

[Bob Vandette]

The changes look ok to me.

I would have preferred that you kept the original logic embedded in the 
higher level functions rather than adding additional static functions.  We’re hoping
to back port this capability to JDK 8 and it just makes the merges more error prone.

I’m also a bit uncomfortable with freeing potentially NULL pointers in print_container_info.
I’ve worked with too many different Linux clibs to trust that they all have the same 
behavior but it is documented on both Linux and Bsd so I guess we’re ok.

Bob.



> On Feb 15, 2018, at 4:37 AM, Robbin Ehn <robbin.ehn at oracle.com> wrote:
> 
> Hi Thomas, thanks for having a look.
> 
> On 2018-02-14 15:06, Thomas Stüfe wrote:
>> 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.
> 
> Sorry bug is:
> https://bugs.openjdk.java.net/browse/JDK-8197408
> 
> Inc here:
> http://cr.openjdk.java.net/~rehn/8197408/v2/inc/webrev/
> Full here:
> http://cr.openjdk.java.net/~rehn/8197408/v2/full/webrev/
> 
> Comments on remarks below:
> 
>> 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.
> 
> We had a debate about this, my opinion was that avoiding the NULL check makes it worth using a ref, but I see I'm pretty lonely in this camp so fixed.
> 
>> -> in available_memory_container():
>> Could we also add "using host value" to the logging in error case as you do in physical_memory_container()?
> 
> Fixed, and tried to improve the logs.
> 
>> -> (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.
> 
> Manually tested this, it seems to work. The -2 gets promoted to unsigned in comparison.
> We go from (jlong)-2 to (julong)ULONG_MAX-1 back to (jlong)-2.
> Not obviously that it will always work. I will not touch that in this changeset.
> 
>> -> Can we:
>> -  st->print("container_type: %s\n", p != NULL ? p : "failed");
>> +  st->print_cr("container_type: %s", p != NULL ? p : "failed");
>> ?
> 
> All of these use print + \n, you want me to just change this one or ?
> I'll rather leave them alone.
> 
>> -> char * OSContainer::container_type() - you free() that value but it is not strdup()ed.
> 
> Thanks for seeing this, copy-paste :)
> 
>> (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* .)
> 
> Changed to const.
> 
> Thanks, Robbin
> 
>> Kind Regards, Thomas
>> On Wed, Feb 14, 2018 at 2:18 PM, Robbin Ehn <robbin.ehn at oracle.com <mailto: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/ <http://cr.openjdk.java.net/~rehn/8197408/webrev/>
>>                        Bug:
>>                        http://cr.openjdk.java.net/~rehn/8197408/webrev/ <http://cr.openjdk.java.net/~rehn/8197408/webrev/>
>>                        Thanks, Robbin