Re: RFR: 8252412: [macos11] system dynamic libraries removed from filesystem [v2]
On Mon, 18 Jan 2021 11:03:06 GMT, Martin Buchholz wrote: >> 8252412: [macos11] system dynamic libraries removed from filesystem > > Martin Buchholz has refreshed the contents of this pull request, and previous > commits have been removed. The incremental views will show differences > compared to the previous content of the PR. Marked as reviewed by valeriep (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/2119
Re: RFR: 8252412: [macos11] system dynamic libraries removed from filesystem [v2]
On Fri, 22 Jan 2021 22:55:22 GMT, Jiangli Zhou wrote: >> Ok, I see Java_sun_security_smartcardio_PlatformPCSC_initialize does dlopen >> using the 'jLibName' (string) obtained from getLibraryName() and throws >> IOException if dlopen fails. The change seems safe enough. >> >> I'm wondering if you want to check the file first then check the parent >> directory if the file does not exist. Not sure if that's a little more >> optimal on older macos, so I'll leave that to you to decide. >> >> For the jtreg test, how about converting Dominik's TestPCSC? As the file is >> a shared for 'unix' platforms, it feels safer at least with some level of >> unit test. Could you please give some more contexts about the >> functionalities associated with PCSC are broken on macos? > > Martin and I had an off-line chat and Martin convinced me that the existing > jtreg tests (such as test/jdk/javax/smartcardio and > test/jdk/sun/security/smartcardio are sufficient) to cover the case. Right, existing tests should cover this already since running the test requires that the library must be loaded. Changes look fine, thanks for fixing this. Kind of surprised the existing filtering didn't catch this as security-related changes and send this to security group for review. - PR: https://git.openjdk.java.net/jdk/pull/2119
Re: RFR: 8252412: [macos11] system dynamic libraries removed from filesystem [v2]
On Fri, 22 Jan 2021 22:56:08 GMT, Jiangli Zhou wrote: >> Martin Buchholz has refreshed the contents of this pull request, and >> previous commits have been removed. The incremental views will show >> differences compared to the previous content of the PR. > > Marked as reviewed by jiangli (Reviewer). Last call for a reviewer familiar with macos or smartcardio. In case of crickets I will submit soon. - PR: https://git.openjdk.java.net/jdk/pull/2119
Re: RFR: 8252412: [macos11] system dynamic libraries removed from filesystem [v2]
On Fri, 22 Jan 2021 22:16:28 GMT, Jiangli Zhou wrote: >> The directory structure is intact - only the file is removed from the >> filesystem. >> More generally, for many frameworks, where there used to be >> >> >> the file is gone, but >> >> >> remains. >> >> I don't think we need a jtreg test, since any functionality associated with >> PCSC is broken on this platform. I added label noreg-other > > Ok, I see Java_sun_security_smartcardio_PlatformPCSC_initialize does dlopen > using the 'jLibName' (string) obtained from getLibraryName() and throws > IOException if dlopen fails. The change seems safe enough. > > I'm wondering if you want to check the file first then check the parent > directory if the file does not exist. Not sure if that's a little more > optimal on older macos, so I'll leave that to you to decide. > > For the jtreg test, how about converting Dominik's TestPCSC? As the file is a > shared for 'unix' platforms, it feels safer at least with some level of unit > test. Could you please give some more contexts about the functionalities > associated with PCSC are broken on macos? Martin and I had an off-line chat and Martin convinced me that the existing jtreg tests (such as test/jdk/javax/smartcardio and test/jdk/sun/security/smartcardio are sufficient) to cover the case. - PR: https://git.openjdk.java.net/jdk/pull/2119
Re: RFR: 8252412: [macos11] system dynamic libraries removed from filesystem [v2]
On Mon, 18 Jan 2021 11:03:06 GMT, Martin Buchholz wrote: >> 8252412: [macos11] system dynamic libraries removed from filesystem > > Martin Buchholz has refreshed the contents of this pull request, and previous > commits have been removed. The incremental views will show differences > compared to the previous content of the PR. Marked as reviewed by jiangli (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/2119
Re: RFR: 8252412: [macos11] system dynamic libraries removed from filesystem [v2]
On Fri, 22 Jan 2021 20:08:48 GMT, Martin Buchholz wrote:
>> src/java.smartcardio/unix/classes/sun/security/smartcardio/PlatformPCSC.java
>> line 132:
>>
>>> 130: // existence of the containing directory instead of the file.
>>> 131: lib = PCSC_FRAMEWORK;
>>> 132: if (new File(lib).getParentFile().isDirectory()) {
>>
>> This is outside of my normal area, could you please explain why checking the
>> existence of the containing directory is equivalent to checking the file
>> here? Does it provide the expected behavior on all unix-like platforms or
>> only macos?
>>
>> Could you please also provide a jtreg test for this change?
>
> The directory structure is intact - only the file is removed from the
> filesystem.
> More generally, for many frameworks, where there used to be
>
>
> the file is gone, but
>
>
> remains.
>
> I don't think we need a jtreg test, since any functionality associated with
> PCSC is broken on this platform. I added label noreg-other
Ok, I see Java_sun_security_smartcardio_PlatformPCSC_initialize does dlopen
using the 'jLibName' (string) obtained from getLibraryName() and throws
IOException if dlopen fails. The change seems safe enough.
I'm wondering if you want to check the file first then check the parent
directory if the file does not exist. Not sure if that's a little more optimal
on older macos, so I'll leave that to you to decide.
For the jtreg test, how about converting Dominik's TestPCSC? As the file is a
shared for 'unix' platforms, it feels safer at least with some level of unit
test. Could you please give some more contexts about the functionalities
associated with PCSC are broken on macos?
-
PR: https://git.openjdk.java.net/jdk/pull/2119
Re: RFR: 8252412: [macos11] system dynamic libraries removed from filesystem [v2]
My github comment was mangled forwarding to core-libs. I suspect the skara
bidirectional mailing list forwarding bot discards lines with leading "/"
.
Instead the bot should pass on unrecognized github comment commands
unmodified.
On Fri, Jan 22, 2021 at 12:12 PM Martin Buchholz
wrote:
> On Fri, 22 Jan 2021 19:46:13 GMT, Jiangli Zhou
> wrote:
>
> >> Martin Buchholz has refreshed the contents of this pull request, and
> previous commits have been removed. The incremental views will show
> differences compared to the previous content of the PR.
> >
> >
> src/java.smartcardio/unix/classes/sun/security/smartcardio/PlatformPCSC.java
> line 132:
> >
> >> 130: // existence of the containing directory instead of the
> file.
> >> 131: lib = PCSC_FRAMEWORK;
> >> 132: if (new File(lib).getParentFile().isDirectory()) {
> >
> > This is outside of my normal area, could you please explain why checking
> the existence of the containing directory is equivalent to checking the
> file here? Does it provide the expected behavior on all unix-like platforms
> or only macos?
> >
> > Could you please also provide a jtreg test for this change?
>
> The directory structure is intact - only the file is removed from the
> filesystem.
> More generally, for many frameworks, where there used to be
>
>
> the file is gone, but
>
>
> remains.
>
> I don't think we need a jtreg test, since any functionality associated
> with PCSC is broken on this platform. I added label noreg-other
>
> -
>
> PR: https://git.openjdk.java.net/jdk/pull/2119
>
Re: RFR: 8252412: [macos11] system dynamic libraries removed from filesystem [v2]
On Fri, 22 Jan 2021 19:46:13 GMT, Jiangli Zhou wrote:
>> Martin Buchholz has refreshed the contents of this pull request, and
>> previous commits have been removed. The incremental views will show
>> differences compared to the previous content of the PR.
>
> src/java.smartcardio/unix/classes/sun/security/smartcardio/PlatformPCSC.java
> line 132:
>
>> 130: // existence of the containing directory instead of the file.
>> 131: lib = PCSC_FRAMEWORK;
>> 132: if (new File(lib).getParentFile().isDirectory()) {
>
> This is outside of my normal area, could you please explain why checking the
> existence of the containing directory is equivalent to checking the file
> here? Does it provide the expected behavior on all unix-like platforms or
> only macos?
>
> Could you please also provide a jtreg test for this change?
The directory structure is intact - only the file is removed from the
filesystem.
More generally, for many frameworks, where there used to be
the file is gone, but
remains.
I don't think we need a jtreg test, since any functionality associated with
PCSC is broken on this platform. I added label noreg-other
-
PR: https://git.openjdk.java.net/jdk/pull/2119
Re: RFR: 8252412: [macos11] system dynamic libraries removed from filesystem [v2]
On Mon, 18 Jan 2021 11:03:06 GMT, Martin Buchholz wrote:
>> 8252412: [macos11] system dynamic libraries removed from filesystem
>
> Martin Buchholz has refreshed the contents of this pull request, and previous
> commits have been removed. The incremental views will show differences
> compared to the previous content of the PR.
src/java.smartcardio/unix/classes/sun/security/smartcardio/PlatformPCSC.java
line 132:
> 130: // existence of the containing directory instead of the file.
> 131: lib = PCSC_FRAMEWORK;
> 132: if (new File(lib).getParentFile().isDirectory()) {
This is outside of my normal area, could you please explain why checking the
existence of the containing directory is equivalent to checking the file here?
Does it provide the expected behavior on all unix-like platforms or only macos?
Could you please also provide a jtreg test for this change?
-
PR: https://git.openjdk.java.net/jdk/pull/2119
Re: RFR: 8252412: [macos11] system dynamic libraries removed from filesystem [v2]
> 8252412: [macos11] system dynamic libraries removed from filesystem Martin Buchholz has refreshed the contents of this pull request, and previous commits have been removed. The incremental views will show differences compared to the previous content of the PR. The pull request contains one new commit since the last revision: JDK-8252412: [macos11] system dynamic libraries removed from filesystem - Changes: - all: https://git.openjdk.java.net/jdk/pull/2119/files - new: https://git.openjdk.java.net/jdk/pull/2119/files/578329f0..85fdffde Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=2119=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=2119=00-01 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/2119.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2119/head:pull/2119 PR: https://git.openjdk.java.net/jdk/pull/2119
