Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
333433002:
Rezip tool used to modify the APK. (Closed)
|
Created:
6 years, 6 months ago by Anton Modified:
6 years, 6 months ago CC:
chromium-reviews, klundberg+watch_chromium.org, bulach+watch_chromium.org, yfriedman+watch_chromium.org, ilevy-cc_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionRezip tool used to modify the APK.
BUG=
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=278423
Patch Set 1 #
Total comments: 42
Patch Set 2 : Changes based on agl's review #Patch Set 3 : Fix logic error in review changes #
Total comments: 20
Patch Set 4 : Changes based on simonb's review #
Total comments: 27
Patch Set 5 : Update for Marcus' review #Patch Set 6 : Update for Ross' comments #
Total comments: 2
Patch Set 7 : Update to the usage message #
Messages
Total messages: 21 (0 generated)
agl for minizip DEPS simonb to review bulach as committer
https://codereview.chromium.org/333433002/diff/1/build/android/rezip/rezip.cc File build/android/rezip/rezip.cc (right): https://codereview.chromium.org/333433002/diff/1/build/android/rezip/rezip.cc... build/android/rezip/rezip.cc:6: // file and outputs a new zip file applying various transforms. The tool "after" before "applying". https://codereview.chromium.org/333433002/diff/1/build/android/rezip/rezip.cc... build/android/rezip/rezip.cc:7: // is used in the Android Chromium build process to modify and APK file s/and/an/ https://codereview.chromium.org/333433002/diff/1/build/android/rezip/rezip.cc... build/android/rezip/rezip.cc:27: const int kMaxFilenameInZip = 256; static for these? https://codereview.chromium.org/333433002/diff/1/build/android/rezip/rezip.cc... build/android/rezip/rezip.cc:61: if (bytes == -1) { unzReadCurrentFile returns negative numbers for error, but not always -1. (E.g. UNZ_CRCERROR is -105.) https://codereview.chromium.org/333433002/diff/1/build/android/rezip/rezip.cc... build/android/rezip/rezip.cc:76: } while (bytes != 0); it looks like this do { } while loop should be a for(;;) loop because bytes != 0 is checked in the middle. https://codereview.chromium.org/333433002/diff/1/build/android/rezip/rezip.cc... build/android/rezip/rezip.cc:83: out_info.tmz_date.tm_sec = in_info.tmu_date.tm_sec; does assigning the whole struct as one not work? https://codereview.chromium.org/333433002/diff/1/build/android/rezip/rezip.cc... build/android/rezip/rezip.cc:143: std::string filename_str = filename; const for these? https://codereview.chromium.org/333433002/diff/1/build/android/rezip/rezip.cc... build/android/rezip/rezip.cc:152: if (filename_str.substr(0, prefix.size()) != prefix) { this test uses strsub and !=, but the next uses just compare. They should probably both be one or the other. https://codereview.chromium.org/333433002/diff/1/build/android/rezip/rezip.cc... build/android/rezip/rezip.cc:164: size_t last_slash = filename_str.find_last_of('/'); these, and below, could be const if you like - not important. https://codereview.chromium.org/333433002/diff/1/build/android/rezip/rezip.cc... build/android/rezip/rezip.cc:177: if (filename_str.compare(last_slash + 1, libprefix.length(), libprefix) != I think you need to check that libprefix isn't out of range for filename_str. https://codereview.chromium.org/333433002/diff/1/build/android/rezip/rezip.cc... build/android/rezip/rezip.cc:183: std::string linker = "libchromium_android_linker.so"; this isn't mentioned in the comment at the top of the function. Also, this could be out of range for filename_str. Lastly, the second clause should be first because it's much cheaper. https://codereview.chromium.org/333433002/diff/1/build/android/rezip/rezip.cc... build/android/rezip/rezip.cc:208: if (last_slash == std::string::npos && if last_slash equals npos, I'm not sure that the second clause of this conjunction can be false. https://codereview.chromium.org/333433002/diff/1/build/android/rezip/rezip.cc... build/android/rezip/rezip.cc:229: if (padding == 0) { Can padding ever be zero? You might want to be checking for padding == kPageSize. https://codereview.chromium.org/333433002/diff/1/build/android/rezip/rezip.cc... build/android/rezip/rezip.cc:231: } maybe if (INT_MAX - size < padding) { abort(); } In case the file size gets near 2GB? https://codereview.chromium.org/333433002/diff/1/build/android/rezip/rezip.cc... build/android/rezip/rezip.cc:251: char in_filename[kMaxFilenameInZip + 1]; you add one to the size as if you were trying to ensure that it was always NUL terminated, but you don't set the last byte to NUL. https://codereview.chromium.org/333433002/diff/1/build/android/rezip/rezip.cc... build/android/rezip/rezip.cc:299: } while (true); use for(;;) rather than do { } while (true). https://codereview.chromium.org/333433002/diff/1/build/android/rezip/rezip.cc... build/android/rezip/rezip.cc:326: char in_filename[kMaxFilenameInZip + 1]; ditto about the NUL termination. https://codereview.chromium.org/333433002/diff/1/build/android/rezip/rezip.cc... build/android/rezip/rezip.cc:347: out_filename = in_filename; this else can be removed. https://codereview.chromium.org/333433002/diff/1/build/android/rezip/rezip.cc... build/android/rezip/rezip.cc:442: } while (true); use for(;;) rather than do { } while(true). https://codereview.chromium.org/333433002/diff/1/build/android/rezip/rezip.cc... build/android/rezip/rezip.cc:471: // as data descriptors are complete redundant information. s/complete//
Addressed in Patch Set 2 https://codereview.chromium.org/333433002/diff/1/build/android/rezip/rezip.cc File build/android/rezip/rezip.cc (right): https://codereview.chromium.org/333433002/diff/1/build/android/rezip/rezip.cc... build/android/rezip/rezip.cc:6: // file and outputs a new zip file applying various transforms. The tool On 2014/06/11 17:52:40, agl wrote: > "after" before "applying". Done. https://codereview.chromium.org/333433002/diff/1/build/android/rezip/rezip.cc... build/android/rezip/rezip.cc:7: // is used in the Android Chromium build process to modify and APK file On 2014/06/11 17:52:41, agl wrote: > s/and/an/ Done. https://codereview.chromium.org/333433002/diff/1/build/android/rezip/rezip.cc... build/android/rezip/rezip.cc:27: const int kMaxFilenameInZip = 256; On 2014/06/11 17:52:40, agl wrote: > static for these? Why? compile time constants are better. https://codereview.chromium.org/333433002/diff/1/build/android/rezip/rezip.cc... build/android/rezip/rezip.cc:61: if (bytes == -1) { On 2014/06/11 17:52:40, agl wrote: > unzReadCurrentFile returns negative numbers for error, but not always -1. (E.g. > UNZ_CRCERROR is -105.) Done. https://codereview.chromium.org/333433002/diff/1/build/android/rezip/rezip.cc... build/android/rezip/rezip.cc:76: } while (bytes != 0); On 2014/06/11 17:52:41, agl wrote: > it looks like this do { } while loop should be a for(;;) loop because bytes != 0 > is checked in the middle. Changed to while (true) https://codereview.chromium.org/333433002/diff/1/build/android/rezip/rezip.cc... build/android/rezip/rezip.cc:83: out_info.tmz_date.tm_sec = in_info.tmu_date.tm_sec; On 2014/06/11 17:52:41, agl wrote: > does assigning the whole struct as one not work? No, they are different types. tm_zip v. tm_unz https://codereview.chromium.org/333433002/diff/1/build/android/rezip/rezip.cc... build/android/rezip/rezip.cc:143: std::string filename_str = filename; On 2014/06/11 17:52:40, agl wrote: > const for these? Done. https://codereview.chromium.org/333433002/diff/1/build/android/rezip/rezip.cc... build/android/rezip/rezip.cc:152: if (filename_str.substr(0, prefix.size()) != prefix) { On 2014/06/11 17:52:41, agl wrote: > this test uses strsub and !=, but the next uses just compare. They should > probably both be one or the other. Changed to use compare https://codereview.chromium.org/333433002/diff/1/build/android/rezip/rezip.cc... build/android/rezip/rezip.cc:164: size_t last_slash = filename_str.find_last_of('/'); On 2014/06/11 17:52:41, agl wrote: > these, and below, could be const if you like - not important. Done. https://codereview.chromium.org/333433002/diff/1/build/android/rezip/rezip.cc... build/android/rezip/rezip.cc:177: if (filename_str.compare(last_slash + 1, libprefix.length(), libprefix) != On 2014/06/11 17:52:41, agl wrote: > I think you need to check that libprefix isn't out of range for filename_str. compare goes until libprefix.length() or the end of the string, so you can't go out of range. last_slash + 1 could be out of range except we already checked that the string ends in ".so". https://codereview.chromium.org/333433002/diff/1/build/android/rezip/rezip.cc... build/android/rezip/rezip.cc:183: std::string linker = "libchromium_android_linker.so"; On 2014/06/11 17:52:41, agl wrote: > this isn't mentioned in the comment at the top of the function. Done > Also, this could be out of range for filename_str. Same as in the previous case. > Lastly, the second clause should be first because it's much cheaper. Done https://codereview.chromium.org/333433002/diff/1/build/android/rezip/rezip.cc... build/android/rezip/rezip.cc:208: if (last_slash == std::string::npos && On 2014/06/11 17:52:41, agl wrote: > if last_slash equals npos, I'm not sure that the second clause of this > conjunction can be false. This is really just a fail safe, as IsLibraryFilename() already guarantees that slash is in a safe location. Anyway you are right it should be an ||. https://codereview.chromium.org/333433002/diff/1/build/android/rezip/rezip.cc... build/android/rezip/rezip.cc:229: if (padding == 0) { On 2014/06/11 17:52:41, agl wrote: > Can padding ever be zero? You might want to be checking for padding == > kPageSize. Done. https://codereview.chromium.org/333433002/diff/1/build/android/rezip/rezip.cc... build/android/rezip/rezip.cc:231: } On 2014/06/11 17:52:41, agl wrote: > maybe > > if (INT_MAX - size < padding) { > abort(); > } > > In case the file size gets near 2GB? Changed to: assert(extra_size < kMaxExtraFieldInZip - padding); https://codereview.chromium.org/333433002/diff/1/build/android/rezip/rezip.cc... build/android/rezip/rezip.cc:251: char in_filename[kMaxFilenameInZip + 1]; On 2014/06/11 17:52:41, agl wrote: > you add one to the size as if you were trying to ensure that it was always NUL > terminated, but you don't set the last byte to NUL. This is based on the code in unzip.c which appears to have the same problem. https://codereview.chromium.org/333433002/diff/1/build/android/rezip/rezip.cc... build/android/rezip/rezip.cc:299: } while (true); On 2014/06/11 17:52:41, agl wrote: > use for(;;) rather than do { } while (true). Changed to while (true) https://codereview.chromium.org/333433002/diff/1/build/android/rezip/rezip.cc... build/android/rezip/rezip.cc:326: char in_filename[kMaxFilenameInZip + 1]; On 2014/06/11 17:52:40, agl wrote: > ditto about the NUL termination. Done. https://codereview.chromium.org/333433002/diff/1/build/android/rezip/rezip.cc... build/android/rezip/rezip.cc:347: out_filename = in_filename; On 2014/06/11 17:52:41, agl wrote: > this else can be removed. Done. https://codereview.chromium.org/333433002/diff/1/build/android/rezip/rezip.cc... build/android/rezip/rezip.cc:442: } while (true); On 2014/06/11 17:52:41, agl wrote: > use for(;;) rather than do { } while(true). Changed to while (true) https://codereview.chromium.org/333433002/diff/1/build/android/rezip/rezip.cc... build/android/rezip/rezip.cc:471: // as data descriptors are complete redundant information. On 2014/06/11 17:52:40, agl wrote: > s/complete// Done.
lgtm All nits. https://codereview.chromium.org/333433002/diff/40001/build/android/rezip/rezi... File build/android/rezip/rezip.cc (right): https://codereview.chromium.org/333433002/diff/40001/build/android/rezip/rezi... build/android/rezip/rezip.cc:31: const int kPageSize = 4096; PAGE_SIZE is defined in limits.h (that said, it's discouraged in favour of sysconf, somewhat painful to use here). https://codereview.chromium.org/333433002/diff/40001/build/android/rezip/rezi... build/android/rezip/rezip.cc:51: static bool CopySubfile(unzFile in_file, Anon namespace rather than static functions? https://codereview.chromium.org/333433002/diff/40001/build/android/rezip/rezi... build/android/rezip/rezip.cc:209: size_t last_slash = filename_str.find_last_of('/'); const? https://codereview.chromium.org/333433002/diff/40001/build/android/rezip/rezi... build/android/rezip/rezip.cc:217: std::string basename_prefix = "crazy."; const? https://codereview.chromium.org/333433002/diff/40001/build/android/rezip/rezi... build/android/rezip/rezip.cc:229: ZPOS64_T pos = unzGetCurrentFileZStreamPos64(in_file); const? https://codereview.chromium.org/333433002/diff/40001/build/android/rezip/rezi... build/android/rezip/rezip.cc:230: int padding = kPageSize - (pos % kPageSize); const? https://codereview.chromium.org/333433002/diff/40001/build/android/rezip/rezi... build/android/rezip/rezip.cc:280: int alignment = pos % kPageSize; const? https://codereview.chromium.org/333433002/diff/40001/build/android/rezip/rezi... build/android/rezip/rezip.cc:295: int next = unzGoToNextFile(in_file); const? https://codereview.chromium.org/333433002/diff/40001/build/android/rezip/rezi... build/android/rezip/rezip.cc:391: int ret = zipOpenNewFileInZip4(out_file, const? https://codereview.chromium.org/333433002/diff/40001/build/android/rezip/rezi... build/android/rezip/rezip.cc:438: int next = unzGoToNextFile(in_file); const?
Addressed in Patch Set #4. Marcus, rubber stamp please. https://codereview.chromium.org/333433002/diff/40001/build/android/rezip/rezi... File build/android/rezip/rezip.cc (right): https://codereview.chromium.org/333433002/diff/40001/build/android/rezip/rezi... build/android/rezip/rezip.cc:31: const int kPageSize = 4096; On 2014/06/13 16:04:13, simonb wrote: > PAGE_SIZE is defined in limits.h (that said, it's discouraged in favour of > sysconf, somewhat painful to use here). Added a comment which explains why you are wrong about this. https://codereview.chromium.org/333433002/diff/40001/build/android/rezip/rezi... build/android/rezip/rezip.cc:51: static bool CopySubfile(unzFile in_file, On 2014/06/13 16:04:13, simonb wrote: > Anon namespace rather than static functions? Style guide says either way is fine. https://codereview.chromium.org/333433002/diff/40001/build/android/rezip/rezi... build/android/rezip/rezip.cc:209: size_t last_slash = filename_str.find_last_of('/'); On 2014/06/13 16:04:13, simonb wrote: > const? Already done. You didn't look at the latest patch. https://codereview.chromium.org/333433002/diff/40001/build/android/rezip/rezi... build/android/rezip/rezip.cc:217: std::string basename_prefix = "crazy."; On 2014/06/13 16:04:13, simonb wrote: > const? Done. https://codereview.chromium.org/333433002/diff/40001/build/android/rezip/rezi... build/android/rezip/rezip.cc:229: ZPOS64_T pos = unzGetCurrentFileZStreamPos64(in_file); On 2014/06/13 16:04:13, simonb wrote: > const? Done. https://codereview.chromium.org/333433002/diff/40001/build/android/rezip/rezi... build/android/rezip/rezip.cc:230: int padding = kPageSize - (pos % kPageSize); On 2014/06/13 16:04:13, simonb wrote: > const? Done. https://codereview.chromium.org/333433002/diff/40001/build/android/rezip/rezi... build/android/rezip/rezip.cc:280: int alignment = pos % kPageSize; On 2014/06/13 16:04:13, simonb wrote: > const? Done. https://codereview.chromium.org/333433002/diff/40001/build/android/rezip/rezi... build/android/rezip/rezip.cc:295: int next = unzGoToNextFile(in_file); On 2014/06/13 16:04:13, simonb wrote: > const? Done. https://codereview.chromium.org/333433002/diff/40001/build/android/rezip/rezi... build/android/rezip/rezip.cc:391: int ret = zipOpenNewFileInZip4(out_file, On 2014/06/13 16:04:13, simonb wrote: > const? Done. https://codereview.chromium.org/333433002/diff/40001/build/android/rezip/rezi... build/android/rezip/rezip.cc:438: int next = unzGoToNextFile(in_file); On 2014/06/13 16:04:13, simonb wrote: > const? Done.
The CQ bit was checked by anton@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/anton@chromium.org/333433002/60001
The CQ bit was unchecked by commit-bot@chromium.org
No LGTM from a valid reviewer yet. Only full committers are accepted. Even if an LGTM may have been provided, it was from a non-committer or a provisional committer, _not_ a full super star committer. See http://www.chromium.org/getting-involved/become-a-committer Note that this has nothing to do with OWNERS files.
lgtm % nits, thanks! perhaps also wait for agl's review? https://codereview.chromium.org/333433002/diff/60001/build/android/rezip/rezi... File build/android/rezip/rezip.cc (right): https://codereview.chromium.org/333433002/diff/60001/build/android/rezip/rezi... build/android/rezip/rezip.cc:33: const int kPageSize = 4096; nit: how about kTargetPageSize? https://codereview.chromium.org/333433002/diff/60001/build/android/rezip/rezi... build/android/rezip/rezip.cc:53: static bool CopySubfile(unzFile in_file, nit: I think it's more common to have an anonymous namespace instead of marking all these functions as static https://codereview.chromium.org/333433002/diff/60001/build/android/rezip/rezi... build/android/rezip/rezip.cc:100: class UnzipCloser { nit: I think these classes are normally named "Scoped..." https://codereview.chromium.org/333433002/diff/60001/build/android/rezip/rezi... build/android/rezip/rezip.cc:187: const std::string linker = "libchromium_android_linker.so"; nit: it may be nicer to get this as a param passed by the build system https://codereview.chromium.org/333433002/diff/60001/build/android/rezip/rezi... build/android/rezip/rezip.cc:245: unzFile in_file = unzOpen(out_zip_filename); nit: this is repeated 315-320, how about rolling into the unzipCloser? https://codereview.chromium.org/333433002/diff/60001/build/android/rezip/rezi... build/android/rezip/rezip.cc:410: /*flagBase */ 0); nit: space after /* https://codereview.chromium.org/333433002/diff/60001/build/android/rezip/rezi... build/android/rezip/rezip.cc:467: bool checkPageAlign = false; nit: check_page_align
LGTM https://codereview.chromium.org/333433002/diff/1/build/android/rezip/rezip.cc File build/android/rezip/rezip.cc (right): https://codereview.chromium.org/333433002/diff/1/build/android/rezip/rezip.cc... build/android/rezip/rezip.cc:27: const int kMaxFilenameInZip = 256; On 2014/06/12 09:31:19, Anton wrote: > Why? compile time constants are better. static doesn't affect whether they are constant or not, but it prevents the symbols from being exported from the compilation unit, which I think you want.
mostly rubber stamp LGMT, but please consider comments below. Unittests would also be nice ;). https://codereview.chromium.org/333433002/diff/60001/build/android/rezip/rezi... File build/android/rezip/rezip.cc (right): https://codereview.chromium.org/333433002/diff/60001/build/android/rezip/rezi... build/android/rezip/rezip.cc:204: static std::string RenameLibrary(const char* in_filename) { Add a comment to describe what this function does. Also, the function name isn't very descriptive - either call it something like RenameLibrariesForCrazyLinker, or add pull out basename_prefix as an argument to make this a generic function which adds a prefix to library files. https://codereview.chromium.org/333433002/diff/60001/build/android/rezip/rezi... build/android/rezip/rezip.cc:224: static int PageAlignCrazyLibrary(const char* in_filename, Comment describing what the function does please. https://codereview.chromium.org/333433002/diff/60001/build/android/rezip/rezi... build/android/rezip/rezip.cc:331: while (true) { I'm not keen on the while (true) with the break in the middle of the loop - could you pull "next" out and loop as: do { ... } while ((err = unzGoToNextFile(in_file)) == UNZ_OK); if (err != UNZ_END_OF_LIST_OF_FILE) { LOG(ERROR) << "failed to go to next file" << in_zip_filename; return false; } return true; Or something similar? https://codereview.chromium.org/333433002/diff/60001/build/android/rezip/rezi... build/android/rezip/rezip.cc:358: inflate = inflate_fun(in_filename); rename inflate_fun to inflate_filename_filter_fun and InflateFun to InflateFilenameFilterFun so that it is obvious that this is a filter for files which are to be inflated, rather than a function which does the inflating. https://codereview.chromium.org/333433002/diff/60001/build/android/rezip/rezi... build/android/rezip/rezip.cc:456: LOG(ERROR) << " <action> is 'inflatealign', 'dropdescriptors' or 'rename'"; Please add some description of what the various actions do. https://codereview.chromium.org/333433002/diff/60001/build/android/rezip/rezi... build/android/rezip/rezip.cc:468: if (strcmp("inflatealign", action) == 0) { The functions themselves are specific to the crazy linker, but the tool and the actions sound very generic. Could you change the actions to specify that this is for crazy linker (e.g., inflatealigncrazylibs, renamecrazylibs)?
https://codereview.chromium.org/333433002/diff/1/build/android/rezip/rezip.cc File build/android/rezip/rezip.cc (right): https://codereview.chromium.org/333433002/diff/1/build/android/rezip/rezip.cc... build/android/rezip/rezip.cc:27: const int kMaxFilenameInZip = 256; On 2014/06/17 17:40:37, agl wrote: > On 2014/06/12 09:31:19, Anton wrote: > > Why? compile time constants are better. > > static doesn't affect whether they are constant or not, but it prevents the > symbols from being exported from the compilation unit, which I think you want. And so does "const". It is in section 7.1.5.1.2 of the spec.
https://codereview.chromium.org/333433002/diff/60001/build/android/rezip/rezi... File build/android/rezip/rezip.cc (right): https://codereview.chromium.org/333433002/diff/60001/build/android/rezip/rezi... build/android/rezip/rezip.cc:33: const int kPageSize = 4096; On 2014/06/17 16:07:08, bulach wrote: > nit: how about kTargetPageSize? changed to PageSizeOnDevice https://codereview.chromium.org/333433002/diff/60001/build/android/rezip/rezi... build/android/rezip/rezip.cc:53: static bool CopySubfile(unzFile in_file, On 2014/06/17 16:07:08, bulach wrote: > nit: I think it's more common to have an anonymous namespace instead of marking > all these functions as static C++ style says either is fine. https://codereview.chromium.org/333433002/diff/60001/build/android/rezip/rezi... build/android/rezip/rezip.cc:100: class UnzipCloser { On 2014/06/17 16:07:08, bulach wrote: > nit: I think these classes are normally named "Scoped..." Done. https://codereview.chromium.org/333433002/diff/60001/build/android/rezip/rezi... build/android/rezip/rezip.cc:187: const std::string linker = "libchromium_android_linker.so"; On 2014/06/17 16:07:08, bulach wrote: > nit: it may be nicer to get this as a param passed by the build system Actually I would prefer it passed the filename of the library, rather than the filename of the linker, but it doesn't and it would be hard to make it otherwise. https://codereview.chromium.org/333433002/diff/60001/build/android/rezip/rezi... build/android/rezip/rezip.cc:204: static std::string RenameLibrary(const char* in_filename) { On 2014/06/18 09:17:03, rmcilroy wrote: > Add a comment to describe what this function does. Also, the function name > isn't very descriptive - either call it something like > RenameLibrariesForCrazyLinker, or add pull out basename_prefix as an argument to > make this a generic function which adds a prefix to library files. Done. https://codereview.chromium.org/333433002/diff/60001/build/android/rezip/rezi... build/android/rezip/rezip.cc:224: static int PageAlignCrazyLibrary(const char* in_filename, On 2014/06/18 09:17:02, rmcilroy wrote: > Comment describing what the function does please. Done. https://codereview.chromium.org/333433002/diff/60001/build/android/rezip/rezi... build/android/rezip/rezip.cc:245: unzFile in_file = unzOpen(out_zip_filename); On 2014/06/17 16:07:08, bulach wrote: > nit: this is repeated 315-320, how about rolling into the unzipCloser? Done. https://codereview.chromium.org/333433002/diff/60001/build/android/rezip/rezi... build/android/rezip/rezip.cc:331: while (true) { On 2014/06/18 09:17:02, rmcilroy wrote: > I'm not keen on the while (true) with the break in the middle of the loop - > could you pull "next" out and loop as: > do { > ... > } while ((err = unzGoToNextFile(in_file)) == UNZ_OK); > > if (err != UNZ_END_OF_LIST_OF_FILE) { > LOG(ERROR) << "failed to go to next file" << in_zip_filename; > return false; > } > return true; > > Or something similar? Earlier reviewer made me remove the do { } while () loop, so I am not putting it back now. https://codereview.chromium.org/333433002/diff/60001/build/android/rezip/rezi... build/android/rezip/rezip.cc:358: inflate = inflate_fun(in_filename); On 2014/06/18 09:17:02, rmcilroy wrote: > rename inflate_fun to inflate_filename_filter_fun and InflateFun to > InflateFilenameFilterFun so that it is obvious that this is a filter for files > which are to be inflated, rather than a function which does the inflating. It is not a filter, so I changed it to InflatePredicateFun https://codereview.chromium.org/333433002/diff/60001/build/android/rezip/rezi... build/android/rezip/rezip.cc:410: /*flagBase */ 0); On 2014/06/17 16:07:08, bulach wrote: > nit: space after /* Done. https://codereview.chromium.org/333433002/diff/60001/build/android/rezip/rezi... build/android/rezip/rezip.cc:456: LOG(ERROR) << " <action> is 'inflatealign', 'dropdescriptors' or 'rename'"; On 2014/06/18 09:17:02, rmcilroy wrote: > Please add some description of what the various actions do. Done. https://codereview.chromium.org/333433002/diff/60001/build/android/rezip/rezi... build/android/rezip/rezip.cc:467: bool checkPageAlign = false; On 2014/06/17 16:07:08, bulach wrote: > nit: check_page_align Done. https://codereview.chromium.org/333433002/diff/60001/build/android/rezip/rezi... build/android/rezip/rezip.cc:468: if (strcmp("inflatealign", action) == 0) { On 2014/06/18 09:17:03, rmcilroy wrote: > The functions themselves are specific to the crazy linker, but the tool and the > actions sound very generic. Could you change the actions to specify that this > is for crazy linker (e.g., inflatealigncrazylibs, renamecrazylibs)? I prefer my non-ugly names.
lgtm https://codereview.chromium.org/333433002/diff/60001/build/android/rezip/rezi... File build/android/rezip/rezip.cc (right): https://codereview.chromium.org/333433002/diff/60001/build/android/rezip/rezi... build/android/rezip/rezip.cc:468: if (strcmp("inflatealign", action) == 0) { On 2014/06/18 13:30:41, Anton wrote: > On 2014/06/18 09:17:03, rmcilroy wrote: > > The functions themselves are specific to the crazy linker, but the tool and > the > > actions sound very generic. Could you change the actions to specify that this > > is for crazy linker (e.g., inflatealigncrazylibs, renamecrazylibs)? > > I prefer my non-ugly names. This is fine with the added usage instructions (although I wouldn't call inflatealign a non-ugly name). https://codereview.chromium.org/333433002/diff/100001/build/android/rezip/rez... File build/android/rezip/rezip.cc (right): https://codereview.chromium.org/333433002/diff/100001/build/android/rezip/rez... build/android/rezip/rezip.cc:477: "lib/*/crazy.lib*.so"; mention that it doesn't rename libchromium_android_linker.so, or add a parameter which enables the build system to specify which libraries to omit from the renaming (as Marcus suggested in line 187, which I also think is a good idea).
On 2014/06/18 14:04:20, rmcilroy wrote: > lgtm > > https://codereview.chromium.org/333433002/diff/60001/build/android/rezip/rezi... > File build/android/rezip/rezip.cc (right): > > https://codereview.chromium.org/333433002/diff/60001/build/android/rezip/rezi... > build/android/rezip/rezip.cc:468: if (strcmp("inflatealign", action) == 0) { > On 2014/06/18 13:30:41, Anton wrote: > > On 2014/06/18 09:17:03, rmcilroy wrote: > > > The functions themselves are specific to the crazy linker, but the tool and > > the > > > actions sound very generic. Could you change the actions to specify that > this > > > is for crazy linker (e.g., inflatealigncrazylibs, renamecrazylibs)? > > > > I prefer my non-ugly names. > > This is fine with the added usage instructions (although I wouldn't call > inflatealign a non-ugly name). > > https://codereview.chromium.org/333433002/diff/100001/build/android/rezip/rez... > File build/android/rezip/rezip.cc (right): > > https://codereview.chromium.org/333433002/diff/100001/build/android/rezip/rez... > build/android/rezip/rezip.cc:477: "lib/*/crazy.lib*.so"; > mention that it doesn't rename libchromium_android_linker.so Done, in patch set #7. > or add a parameter > which enables the build system to specify which libraries to omit from the > renaming (as Marcus suggested in line 187, which I also think is a good idea). See my earlier comment about this
https://codereview.chromium.org/333433002/diff/100001/build/android/rezip/rez... File build/android/rezip/rezip.cc (right): https://codereview.chromium.org/333433002/diff/100001/build/android/rezip/rez... build/android/rezip/rezip.cc:477: "lib/*/crazy.lib*.so"; On 2014/06/18 14:04:20, rmcilroy wrote: > mention that it doesn't rename libchromium_android_linker.so, or add a parameter > which enables the build system to specify which libraries to omit from the > renaming (as Marcus suggested in line 187, which I also think is a good idea). Done.
The CQ bit was checked by anton@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/anton@chromium.org/333433002/120001
On 2014/06/18 15:24:54, Anton wrote: > On 2014/06/18 14:04:20, rmcilroy wrote: > > lgtm > > > > > https://codereview.chromium.org/333433002/diff/60001/build/android/rezip/rezi... > > File build/android/rezip/rezip.cc (right): > > > > > https://codereview.chromium.org/333433002/diff/60001/build/android/rezip/rezi... > > build/android/rezip/rezip.cc:468: if (strcmp("inflatealign", action) == 0) { > > On 2014/06/18 13:30:41, Anton wrote: > > > On 2014/06/18 09:17:03, rmcilroy wrote: > > > > The functions themselves are specific to the crazy linker, but the tool > and > > > the > > > > actions sound very generic. Could you change the actions to specify that > > this > > > > is for crazy linker (e.g., inflatealigncrazylibs, renamecrazylibs)? > > > > > > I prefer my non-ugly names. > > > > This is fine with the added usage instructions (although I wouldn't call > > inflatealign a non-ugly name). > > > > > https://codereview.chromium.org/333433002/diff/100001/build/android/rezip/rez... > > File build/android/rezip/rezip.cc (right): > > > > > https://codereview.chromium.org/333433002/diff/100001/build/android/rezip/rez... > > build/android/rezip/rezip.cc:477: "lib/*/crazy.lib*.so"; > > mention that it doesn't rename libchromium_android_linker.so > > Done, in patch set #7. > > > or add a parameter > > which enables the build system to specify which libraries to omit from the > > renaming (as Marcus suggested in line 187, which I also think is a good idea). > > See my earlier comment about this Well you never actually answered Marcus' suggestion - you mention why it would be difficult to pass the name of the libraries rather than the linker (I can understand this being tricky), but not why you can't pass the name of the loader's .so which is to be excluded from renaming/inflating, which was the suggestion as I understood it. Given that the tool is completely hardcoded to the crazy linker anyway I don't mind too much, but it would be nice to only have a single place to change if we were to rename libchromium_android_linker.so
Message was sent while issue was closed.
Change committed as 278423 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
