JDK-8170120 jimage IOException solution?

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

JDK-8170120 jimage IOException solution?

Michal Vala
Hi,

I'm working on JDK-8170120[1]. I have 2 working solutions, but I'm not happy
with neither one.

That IOException is thrown from jdk.internal.jimage.BasicImageReader, which is
in java.base module. Jimage is implemented in jdk.jlink module
(jdk.tools.jimage.JImageTask).

One solution is new exception NotValidJimageException which can be thrown from
BasicImageReader and catch and handled at JImageTask. Then it's easy to return
proper error message to the output.
Issue is that this new exception has to be public in java.base so de-facto
defining new jdk core api, which of course I don't want to.

Next option is leave there IOException, but give it some known message and then
handle this message in JImageTask.
This work also well, but "parsing" some message from exception is a bit clumsy.


Opinions or other ideas?

[1] - https://bugs.openjdk.java.net/browse/JDK-8170120

--
Michal Vala
OpenJDK QE
Red Hat Czech
Reply | Threaded
Open this post in threaded view
|

Re: JDK-8170120 jimage IOException solution?

Alan Bateman
On 16/02/2018 13:03, Michal Vala wrote:

> Hi,
>
> I'm working on JDK-8170120[1]. I have 2 working solutions, but I'm not
> happy with neither one.
>
> That IOException is thrown from jdk.internal.jimage.BasicImageReader,
> which is in java.base module. Jimage is implemented in jdk.jlink
> module (jdk.tools.jimage.JImageTask).
>
> One solution is new exception NotValidJimageException which can be
> thrown from BasicImageReader and catch and handled at JImageTask. Then
> it's easy to return proper error message to the output.
> Issue is that this new exception has to be public in java.base so
> de-facto defining new jdk core api, which of course I don't want to.
>
> Next option is leave there IOException, but give it some known message
> and then handle this message in JImageTask.
> This work also well, but "parsing" some message from exception is a
> bit clumsy.
Why can't you just catch the IOException and adds the exception message
to the error message that jimage prints?

-Alan
Reply | Threaded
Open this post in threaded view
|

Re: JDK-8170120 jimage IOException solution?

Michal Vala


On 02/16/2018 02:17 PM, Alan Bateman wrote:

> On 16/02/2018 13:03, Michal Vala wrote:
>> Hi,
>>
>> I'm working on JDK-8170120[1]. I have 2 working solutions, but I'm not happy
>> with neither one.
>>
>> That IOException is thrown from jdk.internal.jimage.BasicImageReader, which is
>> in java.base module. Jimage is implemented in jdk.jlink module
>> (jdk.tools.jimage.JImageTask).
>>
>> One solution is new exception NotValidJimageException which can be thrown from
>> BasicImageReader and catch and handled at JImageTask. Then it's easy to return
>> proper error message to the output.
>> Issue is that this new exception has to be public in java.base so de-facto
>> defining new jdk core api, which of course I don't want to.
>>
>> Next option is leave there IOException, but give it some known message and
>> then handle this message in JImageTask.
>> This work also well, but "parsing" some message from exception is a bit clumsy.
> Why can't you just catch the IOException and adds the exception message to the
> error message that jimage prints?

Sure I can do that. However, all output messages are defined at
jimage.properties (jdk.jlink module) file and this won't be possible for this
case. I can't tell whether IOExcaption is caused by wrong jimage file or
something else so all I can do is really just print an exception message. It
also affects behavior of other cases that throws IOException (not sure if bad or
not) that are now catch with Exception and return EXIT_ABNORMAL error code.

So it sounds like another 'wrong' solution to me. Question is what solution is
least wrong and what is desired behavior. I wanted to fix that one specific case
with minimal side-effects.

--
Michal Vala
OpenJDK QE
Red Hat Czech
Reply | Threaded
Open this post in threaded view
|

Re: JDK-8170120 jimage IOException solution?

Alan Bateman
On 16/02/2018 13:35, Michal Vala wrote:
> :
>
> Sure I can do that. However, all output messages are defined at
> jimage.properties (jdk.jlink module) file and this won't be possible
> for this case. I can't tell whether IOExcaption is caused by wrong
> jimage file or something else so all I can do is really just print an
> exception message.
The jimage tool is for troubleshooting purposes, it's not a tool that we
expect many developers to every use directly. It should be okay if the
resource is something like "Unable to open {0}: {1}" where {0} is the
file path specified to the tool, and {1} is the exception message.

-Alan
Reply | Threaded
Open this post in threaded view
|

Re: JDK-8170120 jimage IOException solution?

Michal Vala


On 02/16/2018 02:45 PM, Alan Bateman wrote:

> On 16/02/2018 13:35, Michal Vala wrote:
>> :
>>
>> Sure I can do that. However, all output messages are defined at
>> jimage.properties (jdk.jlink module) file and this won't be possible for this
>> case. I can't tell whether IOExcaption is caused by wrong jimage file or
>> something else so all I can do is really just print an exception message.
> The jimage tool is for troubleshooting purposes, it's not a tool that we expect
> many developers to every use directly. It should be okay if the resource is
> something like "Unable to open {0}: {1}" where {0} is the file path specified to
> the tool, and {1} is the exception message.

That sounds reasonable. Here's the webrev:
http://cr.openjdk.java.net/~shade/8170120/webrev.01/

It fixes JImageVerifyTest so I've removed it from ProblemList.


note: JImageExtractTest still failing on not-empty-dir(JDK-8170114) and
JImageListTest fails on simplest list test. I'll take a look at it later.

--
Michal Vala
OpenJDK QE
Red Hat Czech
Reply | Threaded
Open this post in threaded view
|

Re: JDK-8170120 jimage IOException solution?

Alan Bateman
On 16/02/2018 14:39, Michal Vala wrote:

> :
>
> That sounds reasonable. Here's the webrev:
> http://cr.openjdk.java.net/~shade/8170120/webrev.01/
>
> It fixes JImageVerifyTest so I've removed it from ProblemList.
>
>
> note: JImageExtractTest still failing on not-empty-dir(JDK-8170114)
> and JImageListTest fails on simplest list test. I'll take a look at it
> later.
>
The changes look good, I just wonder if it would be better to specify
{0} as file rather than file.getName(). Just thinking about a script
running this tool failing because the file path is wrong, I think it
could be useful to have it in error.

-Alan

Reply | Threaded
Open this post in threaded view
|

Re: JDK-8170120 jimage IOException solution?

Michal Vala


On 02/17/2018 08:17 AM, Alan Bateman wrote:

> On 16/02/2018 14:39, Michal Vala wrote:
>> :
>>
>> That sounds reasonable. Here's the webrev:
>> http://cr.openjdk.java.net/~shade/8170120/webrev.01/
>>
>> It fixes JImageVerifyTest so I've removed it from ProblemList.
>>
>>
>> note: JImageExtractTest still failing on not-empty-dir(JDK-8170114) and
>> JImageListTest fails on simplest list test. I'll take a look at it later.
>>
> The changes look good, I just wonder if it would be better to specify {0} as
> file rather than file.getName(). Just thinking about a script running this tool
> failing because the file path is wrong, I think it could be useful to have it in
> error.
Yes, true. Attached.

>
> -Alan
>

--
Michal Vala
OpenJDK QE
Red Hat Czech

jdk.patch (1K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: JDK-8170120 jimage IOException solution?

Alan Bateman
On 19/02/2018 12:09, Michal Vala wrote:
>
>
> Yes, true. Attached.
This looks good except the "does not exist" case which needs this change:

-                throw TASK_HELPER.newBadArgs("err.not.a.jimage",
file.getName());
+                throw TASK_HELPER.newBadArgs("err.not.a.jimage", file);

If you agree then I will include it in your patch and push it.

-Alan
Reply | Threaded
Open this post in threaded view
|

Re: JDK-8170120 jimage IOException solution?

Michal Vala


On 02/19/2018 02:07 PM, Alan Bateman wrote:

> On 19/02/2018 12:09, Michal Vala wrote:
>>
>>
>> Yes, true. Attached.
> This looks good except the "does not exist" case which needs this change:
>
> -                throw TASK_HELPER.newBadArgs("err.not.a.jimage", file.getName());
> +                throw TASK_HELPER.newBadArgs("err.not.a.jimage", file);
>
> If you agree then I will include it in your patch and push it.

yes please. thanks


--
Michal Vala
OpenJDK QE
Red Hat Czech