RFR(xxs): 8203680: os::stat() on Posix platform does not need to copy input path

classic Classic list List threaded Threaded
7 messages Options
Reply | Threaded
Open this post in threaded view
|

RFR(xxs): 8203680: os::stat() on Posix platform does not need to copy input path

Thomas Stüfe-2
Hi,

may I get reviews for this tiny cleanup/fix.

Bug: https://bugs.openjdk.java.net/browse/JDK-8203680
patch: http://cr.openjdk.java.net/~stuefe/webrevs/8203680-os-stat-posix-should-not-create-copy/webrev.00/webrev/

os::stat() on all Posix platforms copies the input path into a fixed
sized temp buffer, thereby risking truncation, only to call
os::native_path() which is a noop on all Posix platforms.

This patch replaces this with a straight call to stat(2), so no
truncation anymore. Also, it unifies the coding in os_posix.cpp.

Thanks, Thomas
Reply | Threaded
Open this post in threaded view
|

Re: RFR(xxs): 8203680: os::stat() on Posix platform does not need to copy input path

Harold David Seigel
Hi Thomas,

This change looks good.

What regression tests were run against the change?

Thanks, Harold


On 6/17/2018 10:13 AM, Thomas Stüfe wrote:

> Hi,
>
> may I get reviews for this tiny cleanup/fix.
>
> Bug: https://bugs.openjdk.java.net/browse/JDK-8203680
> patch: http://cr.openjdk.java.net/~stuefe/webrevs/8203680-os-stat-posix-should-not-create-copy/webrev.00/webrev/
>
> os::stat() on all Posix platforms copies the input path into a fixed
> sized temp buffer, thereby risking truncation, only to call
> os::native_path() which is a noop on all Posix platforms.
>
> This patch replaces this with a straight call to stat(2), so no
> truncation anymore. Also, it unifies the coding in os_posix.cpp.
>
> Thanks, Thomas

Reply | Threaded
Open this post in threaded view
|

Re: RFR(xxs): 8203680: os::stat() on Posix platform does not need to copy input path

Thomas Stüfe-2
Hi Harold,

jdk-submit ran through fine.

Thank you for the review.

..Thomas


On Mon, Jun 18, 2018 at 2:59 PM, Harold David Seigel
<[hidden email]> wrote:

> Hi Thomas,
>
> This change looks good.
>
> What regression tests were run against the change?
>
> Thanks, Harold
>
>
>
> On 6/17/2018 10:13 AM, Thomas Stüfe wrote:
>>
>> Hi,
>>
>> may I get reviews for this tiny cleanup/fix.
>>
>> Bug: https://bugs.openjdk.java.net/browse/JDK-8203680
>> patch:
>> http://cr.openjdk.java.net/~stuefe/webrevs/8203680-os-stat-posix-should-not-create-copy/webrev.00/webrev/
>>
>> os::stat() on all Posix platforms copies the input path into a fixed
>> sized temp buffer, thereby risking truncation, only to call
>> os::native_path() which is a noop on all Posix platforms.
>>
>> This patch replaces this with a straight call to stat(2), so no
>> truncation anymore. Also, it unifies the coding in os_posix.cpp.
>>
>> Thanks, Thomas
>
>
Reply | Threaded
Open this post in threaded view
|

Re: RFR(xxs): 8203680: os::stat() on Posix platform does not need to copy input path

David Holmes
In reply to this post by Thomas Stüfe-2
Hi Thomas,

Looks fine.

I'd probably document the semantics for native_path in os.hpp rather
than mentioning Posix in os_windows.cpp though.

I assume the system stat function is the same on Linux, BSD, OS X, AIX
and Solaris?

Thanks,
David

On 18/06/2018 12:13 AM, Thomas Stüfe wrote:

> Hi,
>
> may I get reviews for this tiny cleanup/fix.
>
> Bug: https://bugs.openjdk.java.net/browse/JDK-8203680
> patch: http://cr.openjdk.java.net/~stuefe/webrevs/8203680-os-stat-posix-should-not-create-copy/webrev.00/webrev/
>
> os::stat() on all Posix platforms copies the input path into a fixed
> sized temp buffer, thereby risking truncation, only to call
> os::native_path() which is a noop on all Posix platforms.
>
> This patch replaces this with a straight call to stat(2), so no
> truncation anymore. Also, it unifies the coding in os_posix.cpp.
>
> Thanks, Thomas
>
Reply | Threaded
Open this post in threaded view
|

Re: RFR(xxs): 8203680: os::stat() on Posix platform does not need to copy input path

Thomas Stüfe-2
Hi David,

thanks for the review!

new version:

http://cr.openjdk.java.net/~stuefe/webrevs/8203680-os-stat-posix-should-not-create-copy/webrev.01/webrev/

I moved the comment from os_posix.cpp to os.hpp, nothing else changed.

Yes, I'd assume stat(2) works the same on all platforms. In any case,
my patch does not change this - the code calling stat() was equal on
all posix platforms, I just removed the unnecessary argument copy.

Thanks, Thomas



On Tue, Jun 19, 2018 at 9:00 AM, David Holmes <[hidden email]> wrote:

> Hi Thomas,
>
> Looks fine.
>
> I'd probably document the semantics for native_path in os.hpp rather than
> mentioning Posix in os_windows.cpp though.
>
> I assume the system stat function is the same on Linux, BSD, OS X, AIX and
> Solaris?
>
> Thanks,
> David
>
>
> On 18/06/2018 12:13 AM, Thomas Stüfe wrote:
>>
>> Hi,
>>
>> may I get reviews for this tiny cleanup/fix.
>>
>> Bug: https://bugs.openjdk.java.net/browse/JDK-8203680
>> patch:
>> http://cr.openjdk.java.net/~stuefe/webrevs/8203680-os-stat-posix-should-not-create-copy/webrev.00/webrev/
>>
>> os::stat() on all Posix platforms copies the input path into a fixed
>> sized temp buffer, thereby risking truncation, only to call
>> os::native_path() which is a noop on all Posix platforms.
>>
>> This patch replaces this with a straight call to stat(2), so no
>> truncation anymore. Also, it unifies the coding in os_posix.cpp.
>>
>> Thanks, Thomas
>>
>
Reply | Threaded
Open this post in threaded view
|

Re: RFR(xxs): 8203680: os::stat() on Posix platform does not need to copy input path

David Holmes
On 19/06/2018 5:44 PM, Thomas Stüfe wrote:
> Hi David,
>
> thanks for the review!
>
> new version:
>
> http://cr.openjdk.java.net/~stuefe/webrevs/8203680-os-stat-posix-should-not-create-copy/webrev.01/webrev/
>
> I moved the comment from os_posix.cpp to os.hpp, nothing else changed.

s/os_posix.cpp/os_windows.cpp/ :)

Updated comment looks good - thanks.

> Yes, I'd assume stat(2) works the same on all platforms. In any case,
> my patch does not change this - the code calling stat() was equal on
> all posix platforms, I just removed the unnecessary argument copy.

Well, you're also now relying on the native stat to handle any
excessively long path names ... there may be some subtle differences
there. And you're now accepting path names that are twice as long as
before (MAX_PATH was 2K, but PATH_MAX is 4K).

David

> Thanks, Thomas
>
>
>
> On Tue, Jun 19, 2018 at 9:00 AM, David Holmes <[hidden email]> wrote:
>> Hi Thomas,
>>
>> Looks fine.
>>
>> I'd probably document the semantics for native_path in os.hpp rather than
>> mentioning Posix in os_windows.cpp though.
>>
>> I assume the system stat function is the same on Linux, BSD, OS X, AIX and
>> Solaris?
>>
>> Thanks,
>> David
>>
>>
>> On 18/06/2018 12:13 AM, Thomas Stüfe wrote:
>>>
>>> Hi,
>>>
>>> may I get reviews for this tiny cleanup/fix.
>>>
>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8203680
>>> patch:
>>> http://cr.openjdk.java.net/~stuefe/webrevs/8203680-os-stat-posix-should-not-create-copy/webrev.00/webrev/
>>>
>>> os::stat() on all Posix platforms copies the input path into a fixed
>>> sized temp buffer, thereby risking truncation, only to call
>>> os::native_path() which is a noop on all Posix platforms.
>>>
>>> This patch replaces this with a straight call to stat(2), so no
>>> truncation anymore. Also, it unifies the coding in os_posix.cpp.
>>>
>>> Thanks, Thomas
>>>
>>
Reply | Threaded
Open this post in threaded view
|

Re: RFR(xxs): 8203680: os::stat() on Posix platform does not need to copy input path

Thomas Stüfe-2
Ah, you are right. Still, I assume this would be no issue (not in a
negative way, at least).

..Thomas

On Tue, Jun 19, 2018 at 10:00 AM, David Holmes <[hidden email]> wrote:

> On 19/06/2018 5:44 PM, Thomas Stüfe wrote:
>>
>> Hi David,
>>
>> thanks for the review!
>>
>> new version:
>>
>>
>> http://cr.openjdk.java.net/~stuefe/webrevs/8203680-os-stat-posix-should-not-create-copy/webrev.01/webrev/
>>
>> I moved the comment from os_posix.cpp to os.hpp, nothing else changed.
>
>
> s/os_posix.cpp/os_windows.cpp/ :)
>
> Updated comment looks good - thanks.
>
>> Yes, I'd assume stat(2) works the same on all platforms. In any case,
>> my patch does not change this - the code calling stat() was equal on
>> all posix platforms, I just removed the unnecessary argument copy.
>
>
> Well, you're also now relying on the native stat to handle any excessively
> long path names ... there may be some subtle differences there. And you're
> now accepting path names that are twice as long as before (MAX_PATH was 2K,
> but PATH_MAX is 4K).
>
> David
>
>
>> Thanks, Thomas
>>
>>
>>
>> On Tue, Jun 19, 2018 at 9:00 AM, David Holmes <[hidden email]>
>> wrote:
>>>
>>> Hi Thomas,
>>>
>>> Looks fine.
>>>
>>> I'd probably document the semantics for native_path in os.hpp rather than
>>> mentioning Posix in os_windows.cpp though.
>>>
>>> I assume the system stat function is the same on Linux, BSD, OS X, AIX
>>> and
>>> Solaris?
>>>
>>> Thanks,
>>> David
>>>
>>>
>>> On 18/06/2018 12:13 AM, Thomas Stüfe wrote:
>>>>
>>>>
>>>> Hi,
>>>>
>>>> may I get reviews for this tiny cleanup/fix.
>>>>
>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8203680
>>>> patch:
>>>>
>>>> http://cr.openjdk.java.net/~stuefe/webrevs/8203680-os-stat-posix-should-not-create-copy/webrev.00/webrev/
>>>>
>>>> os::stat() on all Posix platforms copies the input path into a fixed
>>>> sized temp buffer, thereby risking truncation, only to call
>>>> os::native_path() which is a noop on all Posix platforms.
>>>>
>>>> This patch replaces this with a straight call to stat(2), so no
>>>> truncation anymore. Also, it unifies the coding in os_posix.cpp.
>>>>
>>>> Thanks, Thomas
>>>>
>>>
>