Closed
Bug 959047
Opened 10 years ago
Closed 10 years ago
nsIZipWriter: support alignment for stored file
Categories
(Core :: Networking: JAR, defect)
Core
Networking: JAR
Tracking
()
RESOLVED
FIXED
mozilla29
People
(Reporter: swu, Assigned: swu)
References
Details
Attachments
(3 files, 4 obsolete files)
948 bytes,
patch
|
Details | Diff | Splinter Review | |
4.03 KB,
patch
|
bugzilla
:
review+
|
Details | Diff | Splinter Review |
10.10 KB,
patch
|
bugzilla
:
review+
|
Details | Diff | Splinter Review |
For bug 945152 comment 47, we need to suport adding stored(uncompressed) zip file to package, which the aligned to specified size.
Assignee | ||
Comment 1•10 years ago
|
||
This is WIP patch which aligns all stored files in zip to 4096 bytes.
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → swu
Assignee | ||
Comment 2•10 years ago
|
||
Attachment #8360210 -
Attachment is obsolete: true
Assignee | ||
Comment 3•10 years ago
|
||
Attachment #8360284 -
Attachment is obsolete: true
Assignee | ||
Comment 4•10 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)
Comment 5•10 years ago
|
||
how do I add a file without compression into zip?
Assignee | ||
Comment 6•10 years ago
|
||
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•10 years ago
|
||
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•10 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•10 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•10 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•10 years ago
|
||
Attachment #8360285 -
Attachment is obsolete: true
Assignee | ||
Comment 12•10 years ago
|
||
Assignee | ||
Updated•10 years ago
|
Attachment #8361688 -
Flags: review?(tglek)
Assignee | ||
Updated•10 years ago
|
Attachment #8361689 -
Flags: review?(tglek)
Updated•10 years ago
|
Attachment #8361688 -
Flags: review?(tglek) → review?(aklotz)
Updated•10 years ago
|
Attachment #8361689 -
Flags: review?(tglek) → review?(aklotz)
Comment 13•10 years ago
|
||
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•10 years ago
|
Attachment #8361689 -
Flags: review?(aklotz) → review+
Assignee | ||
Comment 14•10 years ago
|
||
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•10 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 16•10 years ago
|
||
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+
Assignee | ||
Comment 17•10 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=412dae2ce416
Assignee | ||
Comment 18•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/3b4d1a4f5816 https://hg.mozilla.org/integration/mozilla-inbound/rev/15ed780a0674
Comment 19•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/3b4d1a4f5816 https://hg.mozilla.org/mozilla-central/rev/15ed780a0674
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
You need to log in
before you can comment on or make changes to this bug.
Description
•