959047 - nsIZipWriter: support alignment for stored file

Status: RESOLVED FIXED

(Core :: Networking: JAR, defect)

Description

[Shian-Yow Wu [:swu]]

For bug 945152 comment 47, we need to suport adding stored(uncompressed) zip file to package, which the aligned to specified size.

Comment 1

[Shian-Yow Wu [:swu]]

Attachment: (WIP) Patch: aligntment for stored files.

This is WIP patch which aligns all stored files in zip to 4096 bytes.

Comment 2

[Shian-Yow Wu [:swu]]

Attachment: Patch: aligntment for stored files.

Comment 3

[Shian-Yow Wu [:swu]]

Attachment: Patch: alignment for stored files.

Comment 4

[Shian-Yow Wu [:swu]]

This patch supports alignment for stored files.

Call alignStoredFiles() with alignment size before close(), then data position of all stored files will be aligned with specified size.

For example:
  zip.alignStoredFiles(4096);
  zip.close();

Comment 5

[Yuren [:yurenju] (AFK ~ Oct. 12) (volunteer mode)]

how do I add a file without compression into zip?

Comment 6

[Shian-Yow Wu [:swu]]

Attachment: Example to add stored file and make it aligned

This is an example to add stored file and make it aligned using gaia/build/webapp-zip.js for keyboard app.

$ BUILD_APP_NAME=keyboard USE_LOCAL_XULRUNNER_SDK=1 XULRUNNER_DIRECTORY=~/work/mozilla-central/obj-x86_64-unknown-linux-gnu/dist make install-gaia

Comment 7

[Yuren [:yurenju] (AFK ~ Oct. 12) (volunteer mode)]

Discussed with Shian-Yow and that API is fine to me and can be added in build script easily.

Comment 8

[Shian-Yow Wu [:swu]]

I plan to support a new API as follows, which can set respective alignment size for each file when been added to zip.

void addAlignedEntryStream(in AUTF8String aZipEntry, in PRTime aModTime,
                           in int32_t aCompression, in nsIInputStream aStream,
                           in boolean aQueue, in uint32_t aAlignSize);

This will give more general and flexible support for alignment requirement.

For short term, please use alignStoredFiles() as proposed earlier.

Comment 9

[Shian-Yow Wu [:swu]]

For new API in comment 8, I proposed a Mozilla proprietary extra field in zip header as follows:

   Value        Size        Description
   -----        ----        -----------
   0xAA01       2 bytes     Tag for this "extra" block type
   TSize        2 bytes     Size of the store data
   AlignSize    2 bytes     Alignment size
   Padding      Variable    Padding information for alignment

For more information on extra field in zip, please see:
http://www.pkware.com/documents/casestudies/APPNOTE.TXT

Comment 10

[Shian-Yow Wu [:swu]]

(In reply to Shian-Yow Wu [:shianyow] from comment #9)
>    TSize        2 bytes     Size of the store data
TSize        2 bytes     Size of the store data for this tag (AlignSize + Padding).

Comment 11

[Shian-Yow Wu [:swu]]

Attachment: Part 1: Align stored files by alignStoredFiles()

Comment 12

[Shian-Yow Wu [:swu]]

Attachment: Part 2: xpcshell test for alignStoredFiles()

Comment 13

[(No longer employed by Mozilla) Aaron Klotz]

Comment on attachment 8361688 [details] [diff] [review]
Part 1: Align stored files by alignStoredFiles()

Review of attachment 8361688 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, however I'd like to see it again once the NS_ENSURE_SUCCESS macro has been removed from the patch.

::: modules/libjar/zipwriter/public/nsIZipWriter.idl
@@ +209,5 @@
> +
> +  /**
> +   * Make all stored(uncompressed) files align to given alignment size.
> +   *
> +   * @param aAlignSize is the alignemnt size, valid values from 2 to 32768.

Typo in the spelling of "alignment"

::: modules/libjar/zipwriter/src/nsZipHeader.cpp
@@ +353,5 @@
> +    uint32_t pa_end;
> +
> +    if (aAlignSize < 2 || aAlignSize > 32768) {
> +      return NS_ERROR_INVALID_ARG;
> +    }

I'd suggest adding an assertion here to ensure that aAlignSize is a power of 2.

@@ +382,5 @@
> +    mLocalExtraField = new uint8_t[mLocalFieldLength + pad_size];
> +    memcpy(mLocalExtraField.get(), field, mLocalFieldLength);
> +    // Use 0xFFFF as tag ID to avoid conflict with other IDs.
> +    // For more information, please see:
> +    // http://www.pkware.com/documents/casestudies/APPNOTE.TXT

Which section of this document are you referencing? The "Extensible data fields" section?

::: modules/libjar/zipwriter/src/nsZipWriter.cpp
@@ +771,5 @@
> +        }
> +
> +        // Flush any remaining data before we start.
> +        rv = mStream->Flush();
> +        NS_ENSURE_SUCCESS(rv, rv);

NS_ENSURE_SUCCESS is deprecated for new code. Suggested alternatives have been added to the style guide. Please replace all occurrences of this.

@@ +792,5 @@
> +            if (count < sizeof(buf)) {
> +                read = count;
> +            } else {
> +                read = sizeof(buf);
> +            }

Can you replace this if/else with std::min(count, sizeof(buf)) to make it a bit more readable?

Comment 14

[Shian-Yow Wu [:swu]]

Attachment: Part 1: Align stored files by alignStoredFiles()

Thank you for the review, Aaron.
This patch addressed review comments, and added power of 2 as the alignment size constraint.

Comment 15

[Shian-Yow Wu [:swu]]

(In reply to Shian-Yow Wu [:shianyow] from comment #8)
> I plan to support a new API as follows, which can set respective alignment
> size for each file when been added to zip.
> 
> void addAlignedEntryStream(in AUTF8String aZipEntry, in PRTime aModTime,
>                            in int32_t aCompression, in nsIInputStream
> aStream,
>                            in boolean aQueue, in uint32_t aAlignSize);
> 
> This will give more general and flexible support for alignment requirement.
> 
> For short term, please use alignStoredFiles() as proposed earlier.

Follow up bug 961622 has been filed for this.

Comment 16

[(No longer employed by Mozilla) Aaron Klotz]

Comment on attachment 8362397 [details] [diff] [review]
Part 1: Align stored files by alignStoredFiles()

Review of attachment 8362397 [details] [diff] [review]:
-----------------------------------------------------------------

r=me when nits are addressed.

::: modules/libjar/zipwriter/src/nsZipHeader.cpp
@@ +353,5 @@
> +    uint32_t pa_end;
> +
> +    // Check for range and power of 2.
> +    if (aAlignSize < 2 || aAlignSize > 32768 ||
> +        (aAlignSize & (aAlignSize -1)) != 0) {

Nit: space between - and 1

::: modules/libjar/zipwriter/src/nsZipWriter.cpp
@@ +745,5 @@
> +    nsresult rv;
> +
> +    // Check for range and power of 2.
> +    if (aAlignSize < 2 || aAlignSize > 32768 ||
> +        (aAlignSize & (aAlignSize -1)) != 0) {

Nit: Add a space between - and 1

Comment 17

[Shian-Yow Wu [:swu]]

https://tbpl.mozilla.org/?tree=Try&rev=412dae2ce416

Comment 18

[Shian-Yow Wu [:swu]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3b4d1a4f5816
https://hg.mozilla.org/integration/mozilla-inbound/rev/15ed780a0674

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/3b4d1a4f5816
https://hg.mozilla.org/mozilla-central/rev/15ed780a0674