Request for review (S, UPDATED-3) 7017193: Small memory leak in get_stack_bounds // os::create_stack_guard_pages

[Poonam Bajaj]

Looks good to me. One small suggestion, the variable name of '[stack]' 
string and its length could be made more reader friendly such as its old 
name 'stack_str'.

Thanks,
Poonam

On 2/26/2011 3:39 AM, Dmitry Samersoff wrote:
> Dan,
>
> Updated webrev (spacing changes):
>
> http://cr.openjdk.java.net/~dsamersoff/7017193/webrev.05/
>
> -Dmitry
>
>
>
> On 2011-02-25 23:57, Daniel D. Daugherty wrote:
>> On 2/25/2011 11:57 AM, Dmitry Samersoff wrote:
>>> Hi Everyone,
>>>
>>> Updated webrev:
>>>
>>> http://cr.openjdk.java.net/~dsamersoff/7017193/webrev.04/
>>
>> src/os/linux/vm/os_linux.cpp
>> nit on line 2656: add space after ','
>> nit on line 2666: HotSpot style is 'while (('
>>
>> src/share/vm/runtime/os.cpp
>> nit on line 1299: add space after ','
>> nit on line 1302: HotSpot style is 'while (('
>> nit on line 1302: add spaces around '=' => ' = '
>> nit on line 1324: HotSpot style is 'while ('
>>
>> src/share/vm/runtime/os.hpp
>> No comments.
>>
>>
>>>
>>> Returned sscanf and addressed other comments.
>>>
>>> -Dmitry
>>>
>>>
>>>
>>> On 2011-02-25 01:43, Daniel D. Daugherty wrote:
>>>> On 2/24/2011 2:24 PM, Dmitry Samersoff wrote:
>>>>> [got mailer error for openjdk alias, so resending]
>>>>>
>>>>> Hi Everyone,
>>>>>
>>>>> Here is updated fix:
>>>>>
>>>>> http://cr.openjdk.java.net/~dsamersoff/7017193/webrev.03/
>>>>
>>>> src/os/linux/vm/os_linux.cpp
>>>> line 2662: should be /proc/self/maps
>>>>
>>>> line 2663: literal '128' should be 'sizeof(buf)'
>>>>
>>>> line 2664: original logic looks for line ending with "[stack]"
>>>> and your new logic matches a line with "[stack]" anywhere.
>>>>
>>>> line 2666: You assume that you got a number here. The original
>>>> code would have returned false.
>>>>
>>>> line 2667: You assert if '-' is not after the first unsigned
>>>> number translated. The orignal code would have returned
>>>> false in case of such a mis-format.
>>>>
>>>> line 2668: You assume that 'np' is valid here (in product).
>>>> The original code would have returned false.
>>>> The second param to the second strtoul() call should be
>>>> NULL or (char*) NULL.
>>>>
>>>>
>>>> src/share/vm/runtime/os.cpp
>>>> lines 1296-7 - space after '//'
>>>>
>>>> "const" on bsize parameter?
>>>>
>>>> line 1300: please consider adding this comment:
>>>> // read until EOF, EOL or buf is full
>>>>
>>>> line 1305: please consider the following comment between lines
>>>> 1306 and 1307 instead:
>>>> // EOL reached so ignore EOL character and return
>>>>
>>>> lines 1313-4: please consider the following comment between lines
>>>> 1315 and 1316 instead:
>>>> // EOF reached. If we read chars before EOF, return them and
>>>> // return EOF on next call. Otherwise, return EOF.
>>>>
>>>> line 1321: please move trailing ";" to next line, e.g.:
>>>>
>>>> while( read(fd, &ch, 1) == 1 && ch != '\n' )
>>>> /* do nothing */ ;
>>>>
>>>> line 1323: please consider this comment instead:
>>>>
>>>> // Return initial part of line that fits in buf.
>>>> // If we reached EOF, it will be returned on next call.
>>>>
>>>>
>>>> src/share/vm/runtime/os.hpp
>>>> "const" on bsize parameter?
>>>>
>>>> Why add blank lines on 725-6?
>>>>
>>>>
>>>>>
>>>>> I introduce a new convenience function primary designed to read
>>>>> variety of /proc files. It reads first N characters of line and then
>>>>> skip until eol.
>>>>>
>>>>> Than get_stack_bounds() is changed to use it instead of stdio
>>>>> getline();
>>>>>
>>>>> -Dmitry
>>>>>
>>>
>>>
>
>

-- 
Best regards, Poonam

Sun, an Oracle company
Sun, an Oracle Company
Poonam Bajaj | Staff Engineer
Phone: +66937451 <tel:+66937451> | Mobile: +9844511366 <tel:+9844511366>
JVM Sustaining Engineering
| Bangalore
Green Oracle <http://www.oracle.com/commitment> Oracle is committed to 
developing practices and products that help protect the environment

-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.openjdk.java.net/pipermail/hotspot-runtime-dev/attachments/20110228/dd9acc8a/attachment.html 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: sun-oracle-logo.gif
Type: image/gif
Size: 2088 bytes
Desc: not available
Url : http://mail.openjdk.java.net/pipermail/hotspot-runtime-dev/attachments/20110228/dd9acc8a/attachment.gif 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: green-for-email-sig_0.gif
Type: image/gif
Size: 356 bytes
Desc: not available
Url : http://mail.openjdk.java.net/pipermail/hotspot-runtime-dev/attachments/20110228/dd9acc8a/attachment-0001.gif