RFR JDK-8170114 jimage extract to not an empty directory overwrites content of the directory

[Michal Vala]

On 02/10/2018 11:25 AM, Alan Bateman wrote:
> 
> 
> On 10/02/2018 00:04, mandy chung wrote:
>> On 2/9/18 7:23 AM, Michal Vala wrote:
>>>
>>> Patch validates output directory before any jimage extracting happen. I've 
>>> moved validation to extra private method as it is few lines of code. I've 
>>> also added proper error message for case when output path is not a directory 
>>> (JImageTask.java#449).
>>>
>>
>> Thanks for looking at JDK-8170114 and JDK-8170120.  I took a look at 
>> http://cr.openjdk.java.net/~shade/8170114/webrev.01/
> Yes, thanks for looking at these issues.
> 
> For `jimage info <not-a-jimage-file>` issue then jimage correctly fails (with a 
> non-zero status) but the IOException isn't converted into an error message as it 
> does with other errors. Yes, it would be good to fix that.
> 
> I think the `jimage extract --dir <existing-file> <jimage-file>` scenario needs 
> discussion. If <existing-file> is a non-directory file then jimage has to fail, 
> I don't expect disagreement on that. For the case where it is an existing 
> directory then the options seem to be:
> 
> 1. Extract into the existing directory (existing JDK 9 and JDK 10 behavior)
> 2. Fail if it's not empty (your patch)
> 3. Fail if it exists (Mandy's mail, the motivation being to keep it consistent 
> with jlink)
> 
> I view `jimage extract --dir <dir>` as being similar to `unzip -d <dir>` so I 
> don't think existing behavior (#1) is incorrect, the only issue is that it 
> silently overrides files whereas unzip will prompt before overriding (unless you 
> specify -o). The `jar` tool, and legacy `tar` tool side with `jimage` and are 
> happy to silently replace existing files.
> 
> What would you think about focusing on the override case instead of disallowing 
> extracting into an existing non-empty directory? I realize this is more work as 
> it means deciding on whether to prompt, warn or fail. It also means thinking 
> about the equivalent of unzip -o to allowing existing files be replaced.

Unzip prompts user for individual files and I'm not sure whether it's a good 
option here. Maybe prompt user at the beginning that directory is not empty and 
there is a risk that files there might be overwritten continue y/n?


-- 
Michal Vala
OpenJDK QE
Red Hat Czech