Code Review request for new "jsign" tool

[Sean Mullan]

Mandy,

I have updated the webrev for the new jsign tool:

http://cr.openjdk.java.net/~mullan/jigsaw/webrevs/jsign/webrev.01/

This should address most of your comments (see below).

Please let me know if it is ok to push this.

On 5/11/11 5:30 PM, Mandy Chung wrote:
> On 05/10/11 10:10, Sean Mullan wrote:
>> -f, --signedmodulefile <File: path> File name of signed module file
>
> This is a question more to the CLI guideline/convention our tools should follow.
>
> Should an option name with multiple words uses '-' to separate each word?
> "--signed-module-file" vs "--signedmodulefile"?
>
> --output (-o) is the typical option name for specifying the output from a CLI.
> Should we use --output for the jsign command? I like the explicit
> --signed-module-file option name. It might be good to have a consistent
> convention for all of our new tools to follow.

For now, I have left the options as they are, but I agree we need to define a 
more consistent option naming syntax across all of the new jigsaw tools.

>> webrev: http://cr.openjdk.java.net/~mullan/jigsaw/webrevs/jsign/webrev.00/
>
> Signer.java
> L255: what if the input module file is a signed module file? Should it
> output an error?

Yes, this is fixed. See lines 230-231.

> L247, 248: constant 32 and 12 - is it worth defining these constants
> in the ModuleFileFormat class?

Fixed. There are new constants defined in ModuleFileFormat.

> ModuleFileFormat.java
> L630-637: I believe if noExtract is true, destination will be null; otherwise,
> destination is non-null. Looks like you don't need the noExtract flag and
> perhaps replaced with needExtract method? Just a thought.

I have not done this. Since there are several public methods that are affected 
by this flag, I think it is better (and the code is easier to understand) to use 
a flag to denote this behavior.

> L642-643: This assumes the 'sections' field doesn't count the signature
> section. I assume you will take out the commented line before you push
> the push.

In the new webrev, the sections field has been removed from the Module File. It 
isn't strictly necessary, and avoids the signing issue that I described below.

Mark will eventually be sending out an updated Module File Format specification 
with this change.

>
> Other than that, looks good.

Thanks.

I also fixed several unit test failures that were failing on Windows due to the 
usage of FileChannel.map [1]. I replaced these with FileChannel.read (and 
non-direct ByteBuffers). I did not see any noticeable performance degradation, 
but we will probably need to do some tests with larger module files to see if it 
makes any difference. Also, these calls are only made when packaging the module 
files, and not while reading.

--Sean

[1] http://mail.openjdk.java.net/pipermail/jigsaw-dev/2011-June/001366.html

>
>>
>> I encountered one issue with the module-file format [2] which should be
>> addressed. Ideally, when a signature is generated over an existing module
>> file, none of the contents of that module file should be modified. However,
>> there is one field (the sections field in the module file header) that breaks
>> that rule, because the signature itself is a section, and therefore the number
>> of sections needs to be incremented by one. It may be possible to do that, but
>> it would result in the code being much more complex. Thus, I would like to
>> propose that this field be changed to be the number of sections following the
>> module-info section (or the signature section if included), i.e. the number of
>> sections in the "rest" of the module. This would not affect the Reader
>> implementation, as it only uses this field to determine how many sections it
>> needs to read in the rest of the module.
>>
>
> Otherwise, you would need to rehash, right? This sounds a good suggestion to me.
>
> Mandy