nsIZipWriter: support alignment for stored file

RESOLVED FIXED in mozilla29

Status

()

Core
Networking: JAR
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: swu, Assigned: swu)

Tracking

unspecified
mozilla29
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 4 obsolete attachments)

(Assignee)

Description

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

Updated

4 years ago
Blocks: 957497
(Assignee)

Comment 1

4 years ago
Created attachment 8360210 [details] [diff] [review]
(WIP) Patch: aligntment for stored files.

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

Updated

4 years ago
Assignee: nobody → swu
(Assignee)

Comment 2

4 years ago
Created attachment 8360284 [details] [diff] [review]
Patch: aligntment for stored files.
Attachment #8360210 - Attachment is obsolete: true
(Assignee)

Comment 3

4 years ago
Created attachment 8360285 [details] [diff] [review]
Patch: alignment for stored files.
Attachment #8360284 - Attachment is obsolete: true
(Assignee)

Comment 4

4 years ago
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();
Flags: needinfo?(yurenju.mozilla)
how do I add a file without compression into zip?
(Assignee)

Comment 6

4 years ago
Created attachment 8360306 [details] [diff] [review]
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
Discussed with Shian-Yow and that API is fine to me and can be added in build script easily.
Flags: needinfo?(yurenju.mozilla)
(Assignee)

Comment 8

4 years ago
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.
(Assignee)

Comment 9

4 years ago
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
(Assignee)

Comment 10

4 years ago
(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).
(Assignee)

Comment 11

4 years ago
Created attachment 8361688 [details] [diff] [review]
Part 1: Align stored files by alignStoredFiles()
Attachment #8360285 - Attachment is obsolete: true
(Assignee)

Comment 12

4 years ago
Created attachment 8361689 [details] [diff] [review]
Part 2: xpcshell test for alignStoredFiles()
(Assignee)

Updated

4 years ago
Attachment #8361688 - Flags: review?(tglek)
(Assignee)

Updated

4 years ago
Attachment #8361689 - Flags: review?(tglek)

Updated

4 years ago
Attachment #8361688 - Flags: review?(tglek) → review?(aklotz)

Updated

4 years ago
Attachment #8361689 - Flags: review?(tglek) → review?(aklotz)
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?
Attachment #8361688 - Flags: review?(aklotz) → review-

Updated

4 years ago
Attachment #8361689 - Flags: review?(aklotz) → review+
(Assignee)

Comment 14

4 years ago
Created attachment 8362397 [details] [diff] [review]
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.
Attachment #8361688 - Attachment is obsolete: true
Attachment #8362397 - Flags: review?(aklotz)
(Assignee)

Comment 15

4 years ago
(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 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
Attachment #8362397 - Flags: review?(aklotz) → review+
https://hg.mozilla.org/mozilla-central/rev/3b4d1a4f5816
https://hg.mozilla.org/mozilla-central/rev/15ed780a0674
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
You need to log in before you can comment on or make changes to this bug.