RFR: 8146115 - Improve docker container detection and resource configuration usage

[Bob Vandette]

> On Oct 25, 2017, at 2:57 AM, Kim Barrett <kim.barrett at oracle.com> wrote:
> 
>> On Oct 24, 2017, at 10:11 AM, Bob Vandette <bob.vandette at oracle.com> wrote:
>> 
>> 
>>> On Oct 23, 2017, at 12:52 AM, Kim Barrett <kim.barrett at oracle.com> wrote:
>>> 
>>>> On Sep 27, 2017, at 9:20 PM, David Holmes <david.holmes at oracle.com> wrote:
>>>>>> 62     void set_subsystem_path(char *cgroup_path) {
>>>>>> 
>>>>>> If this takes a "const char*" will it save you from casting string literals to "char*" elsewhere?
>>>>> I tried several different ways of declaring the container accessor functions and
>>>>> always ended up with warnings due to scanf not being able to validate arguments
>>>>> since the format string didn’t end up being a string literal.  I originally was using templates
>>>>> and then ended up with the macros.  I tried several different casts but could resolve the problem.
>>>> 
>>>> Sounds like something Kim Barrett should take a look at :)
>>> 
>>> Fortunately, I just happened by.
>>> 
>>> The warnings are because we compile with -Wformat=2, which enables
>>> -Wformat-nonliteral (among other things).
>>> 
>>> Use PRAGMA_FORMAT_NONLITERAL_IGNORED, e.g.
>>> 
>>> PRAGMA_DIAG_PUSH
>>> PRAGMA_FORMAT_NONLITERAL_IGNORED
>>> <function definition>
>>> PRAGMA_DIAG_POP
>>> 
>>> That will silence warnings about sscanf (or anything else!) with a
>>> non-literal format string within that <function definition>.
>> 
>> Thanks but I ended up taking a different approach that resulted in more compact code.
>> 
>> http://cr.openjdk.java.net/~bobv/8146115/webrev.02
> 
> Not a review, just a few more comments in passing.
> 
> ------------------------------------------------------------------------------ 
> src/hotspot/os/linux/osContainer_linux.cpp 
> 150             log_debug(os, container)("Type %s not found in file %s\n",    \
> 151                                      scan_fmt , buf);                     \
> 
> uses buf as path, but buf has been clobbered to contain contents from
> file.
> 
> Similarly for
> 155         log_debug(os, container)("Empty file %s\n", buf);                 \

I fixed these by adding an additional buffer for the read.

> 
> ------------------------------------------------------------------------------
> src/hotspot/os/linux/osContainer_linux.cpp 
> 158       log_debug(os, container)("file not found %s\n", buf);               \
> 
> There are many reasons why fopen might fail, and merging them all into
> a "file not found" message could be quite confusing.  It would be much
> better to report the error from errno.

I added os::strerror(errno) to all failures from fopen to provide more detail.

> 
> ------------------------------------------------------------------------------ 
> src/hotspot/os/linux/osContainer_linux.cpp 
> 
> Something like the following (where the obvious helpers are made up to
> keep this short) would eliminate the macrology.
> 
> PRAGMA_DIAG_PUSH  
> PRAGMA_FORMAT_NONLITERAL_IGNORED
> template<typename T>
> int get_subsystem_file_contents_value(CgroupSubsystem* c,
>                                  const char* filename,
>                                  T* returnval,
>                                  const char* scan_fmt,
> 				  const char* description) {
>  const char* line = get_subsystem_file_line(c, filename);
>  if (line != NULL) {
>    if (sscanf(line, scan_fmt, returnval) == 1) {
>      return 0;
>    } else {
>      report_subsystem_file_contents_parse_error(description, c, filename);
>    }
>  }
>  return OSCONTAINER_ERROR;
> }
> PRAGMA_DIAG_POP
> 
> int subsystem_file_contents_int(CgroupSubsystem* c,
>                                const char* filename,
>                                int* returnval) {
>  return get_subsystem_file_contents_value(c, filename, returnval, "%d", "int");
> }
> 

I originally tried to use a template but ran into the issue of the literal and strings
needed to be handled differently.  I wasn’t sure how to limit the length of the string
but I now see that I can use something like “%1023s”.  I’ll give it a try.

Bob.