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

[Alan Bateman]

Post by 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.

Why can't you just catch the IOException and adds the exception message
to the error message that jimage prints?

-Alan

[Michal Vala]

Post by 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.

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.

[Alan Bateman]

Post by Michal Vala
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

[Michal Vala]

Post by Michal Vala
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.

[Alan Bateman]

Post by Michal Vala
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

[Michal Vala]

Post by Michal Vala
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

[Alan Bateman]

Post by Michal Vala
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

[Michal Vala]

Post by Michal Vala
Yes, true. Attached.

-                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