Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
8873039:
Add an API to unpack Zip files directly from and to file descriptors. (Closed)
|
Created:
9 years ago by Jorge Lucangeli Obes Modified:
9 years ago Reviewers:
satorux1 CC:
chromium-reviews, Paweł Hajdan Jr., Hironori Bono Base URL:
http://git.chromium.org/chromium/src.git@master Visibility:
Public. |
DescriptionAdd an API to unpack Zip files directly from and to file descriptors.
BUG=None
TEST=zip_reader_unittest.cc (compiles into 'unit_tests' binary)
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=114684
Patch Set 1 #
Total comments: 14
Patch Set 2 : Addressed satorux1 comments. Rebased. #Patch Set 3 : Lint fixes. #
Total comments: 6
Patch Set 4 : Added FdWrapper class for testing, removed unused code. #Patch Set 5 : Fixed lint errors. #
Total comments: 10
Patch Set 6 : Style fixes. #
Total comments: 8
Patch Set 7 : More style fixes. #
Messages
Total messages: 18 (0 generated)
Reworked Zip file handling patch that does not need file access by path.
The code looks good. Style nits only: http://codereview.chromium.org/8873039/diff/1/chrome/common/zip_internal.cc File chrome/common/zip_internal.cc (right): http://codereview.chromium.org/8873039/diff/1/chrome/common/zip_internal.cc#n... chrome/common/zip_internal.cc:85: unzFile OpenFdForUnzipping(const int fd) { fd -> zip_fd, to be consistent with the function declaration http://codereview.chromium.org/8873039/diff/1/chrome/common/zip_internal.cc#n... chrome/common/zip_internal.cc:88: return unzOpen2("fd", &zip_funcs); What's "fd"? Is this a dummy file name? Would be nice to add some comment as it looks rather cryptic. http://codereview.chromium.org/8873039/diff/1/chrome/common/zip_internal.h File chrome/common/zip_internal.h (right): http://codereview.chromium.org/8873039/diff/1/chrome/common/zip_internal.h#ne... chrome/common/zip_internal.h:25: unzFile OpenFdForUnzipping(const int zip_fd); const int -> int. We don't use 'const' for parameters of non-pointer/reference types like int. Note that you can use 'int' in the function declaration here, and 'const int' in the function definition in .cc file, but we rarely do this. http://codereview.chromium.org/8873039/diff/1/chrome/common/zip_reader.cc File chrome/common/zip_reader.cc (right): http://codereview.chromium.org/8873039/diff/1/chrome/common/zip_reader.cc#new... chrome/common/zip_reader.cc:85: return Initialize(); OpenInternal() may sound a bit better? http://codereview.chromium.org/8873039/diff/1/chrome/common/zip_reader.h File chrome/common/zip_reader.h (right): http://codereview.chromium.org/8873039/diff/1/chrome/common/zip_reader.h#newc... chrome/common/zip_reader.h:84: bool OpenFd(const int zip_fd); We often use "FromSomething" for naming functions like this, so OpenFromFd() may sound a bit nicer in our code base. const int -> int. http://codereview.chromium.org/8873039/diff/1/chrome/common/zip_reader_unitte... File chrome/common/zip_reader_unittest.cc (right): http://codereview.chromium.org/8873039/diff/1/chrome/common/zip_reader_unitte... chrome/common/zip_reader_unittest.cc:62: int OpenFileRdOnly(FilePath& file) { Please write a brief function comment. http://www.corp.google.com/eng/doc/cppguide.xml?showone=Function_Comments#Fun... Rd -> Read. We eschew abbreviation like this http://www.corp.google.com/eng/doc/cppguide.xml?showone=General_Naming_Rules#... http://codereview.chromium.org/8873039/diff/1/chrome/common/zip_reader_unitte... chrome/common/zip_reader_unittest.cc:64: } We usually have a blank line between function definitions.
Addressed comments http://codereview.chromium.org/8873039/diff/1/chrome/common/zip_internal.cc File chrome/common/zip_internal.cc (right): http://codereview.chromium.org/8873039/diff/1/chrome/common/zip_internal.cc#n... chrome/common/zip_internal.cc:85: unzFile OpenFdForUnzipping(const int fd) { On 2011/12/12 04:54:14, satorux1 wrote: > fd -> zip_fd, to be consistent with the function declaration Done. http://codereview.chromium.org/8873039/diff/1/chrome/common/zip_internal.cc#n... chrome/common/zip_internal.cc:88: return unzOpen2("fd", &zip_funcs); On 2011/12/12 04:54:14, satorux1 wrote: > What's "fd"? Is this a dummy file name? Would be nice to add some comment as it > looks rather cryptic. Done. http://codereview.chromium.org/8873039/diff/1/chrome/common/zip_internal.h File chrome/common/zip_internal.h (right): http://codereview.chromium.org/8873039/diff/1/chrome/common/zip_internal.h#ne... chrome/common/zip_internal.h:25: unzFile OpenFdForUnzipping(const int zip_fd); On 2011/12/12 04:54:14, satorux1 wrote: > const int -> int. We don't use 'const' for parameters of non-pointer/reference > types like int. > > Note that you can use 'int' in the function declaration here, and 'const int' in > the function definition in .cc file, but we rarely do this. Done. http://codereview.chromium.org/8873039/diff/1/chrome/common/zip_reader.cc File chrome/common/zip_reader.cc (right): http://codereview.chromium.org/8873039/diff/1/chrome/common/zip_reader.cc#new... chrome/common/zip_reader.cc:85: return Initialize(); On 2011/12/12 04:54:14, satorux1 wrote: > OpenInternal() may sound a bit better? Done. http://codereview.chromium.org/8873039/diff/1/chrome/common/zip_reader.h File chrome/common/zip_reader.h (right): http://codereview.chromium.org/8873039/diff/1/chrome/common/zip_reader.h#newc... chrome/common/zip_reader.h:84: bool OpenFd(const int zip_fd); On 2011/12/12 04:54:14, satorux1 wrote: > We often use "FromSomething" for naming functions like this, so OpenFromFd() may > sound a bit nicer in our code base. > > const int -> int. Done. http://codereview.chromium.org/8873039/diff/1/chrome/common/zip_reader_unitte... File chrome/common/zip_reader_unittest.cc (right): http://codereview.chromium.org/8873039/diff/1/chrome/common/zip_reader_unitte... chrome/common/zip_reader_unittest.cc:62: int OpenFileRdOnly(FilePath& file) { On 2011/12/12 04:54:14, satorux1 wrote: > Please write a brief function comment. > > http://www.corp.google.com/eng/doc/cppguide.xml?showone=Function_Comments#Fun... > > Rd -> Read. We eschew abbreviation like this > > http://www.corp.google.com/eng/doc/cppguide.xml?showone=General_Naming_Rules#... Done. http://codereview.chromium.org/8873039/diff/1/chrome/common/zip_reader_unitte... chrome/common/zip_reader_unittest.cc:64: } On 2011/12/12 04:54:14, satorux1 wrote: > We usually have a blank line between function definitions. Done.
Addressed satorux1 comments and fixed lint checks.
Please reply to comments by hitting 'Done' button from next time. That'll make it easier to track what's changed. Please fix leaked file descriptors in the test. looks good elsewhere. http://codereview.chromium.org/8873039/diff/10004/chrome/common/zip_reader.h File chrome/common/zip_reader.h (right): http://codereview.chromium.org/8873039/diff/10004/chrome/common/zip_reader.h#... chrome/common/zip_reader.h:159: bool ExtractCurrentEntryToBuffer(const char* buf, const int size); Unused? I guess you wanted to factor out the loop in ExtractCurrentEntry* into this function, but turned out it's not worth doing it, which I agree. :) http://codereview.chromium.org/8873039/diff/10004/chrome/common/zip_reader_un... File chrome/common/zip_reader_unittest.cc (right): http://codereview.chromium.org/8873039/diff/10004/chrome/common/zip_reader_un... chrome/common/zip_reader_unittest.cc:100: ASSERT_TRUE(reader.OpenFromFd(zip_fd)); We should close the fd, but adding close(fd) here will leak the fd if ASSERT fails. I'd introduce a wrapper class to ensure fd is closed, like FdWrapper wrapper(test_zip_file_, FdWrapper::READ_ONLY); ASSERT_TRUE(reader.OpenFromFd(wrapper.fd()); http://codereview.chromium.org/8873039/diff/10004/chrome/common/zip_reader_un... chrome/common/zip_reader_unittest.cc:138: int zip_fd = OpenFileReadOnly(test_zip_file_); ditto. the fd is leaked.
Jorge, hbono@ pointed out there was a GYP flag named "use_system_zlib" and we use the system zlib (libz.so) on Linux. The change will make it impossible to use the system zlib. I'm not sure if this is OK. He suggested to bring this up on chromium-dev. See third_party/zlib/zlib.gyp for details.
Good news. Looking at the changes in third_party/zlib, the patch just adds two functions without modifying the existing functions. I think we can add these functions in zip_internal.cc, then we don't need to touch third_party/zlib. What do you think?
On Mon, Dec 12, 2011 at 10:09 PM, <satorux@chromium.org> wrote: > Good news. Looking at the changes in third_party/zlib, the patch just adds > two > functions without modifying the existing functions. > > I think we can add these functions in zip_internal.cc, then we don't need to > touch third_party/zlib. What do you think? I think it's a very good suggestion, thanks! It's always better not to touch stuff in third_party. I'll upload a revised version.
FWIW, I take back my comment saying "The change will make it impossible to use the system zlib." Looking at http://src.chromium.org/viewvc/chrome/trunk/src/third_party/zlib/zlib.gyp?vie..., files under third_party/zlib/contrib are compiled even if use_system_zlib is specified, so touching files there is actually acceptable, though it'd be nicer not to touch them anyway. :)
http://codereview.chromium.org/8873039/diff/10004/chrome/common/zip_reader.h File chrome/common/zip_reader.h (right): http://codereview.chromium.org/8873039/diff/10004/chrome/common/zip_reader.h#... chrome/common/zip_reader.h:159: bool ExtractCurrentEntryToBuffer(const char* buf, const int size); On 2011/12/13 05:23:08, satorux1 wrote: > Unused? I guess you wanted to factor out the loop in ExtractCurrentEntry* into > this function, but turned out it's not worth doing it, which I agree. :) Done. http://codereview.chromium.org/8873039/diff/10004/chrome/common/zip_reader_un... File chrome/common/zip_reader_unittest.cc (right): http://codereview.chromium.org/8873039/diff/10004/chrome/common/zip_reader_un... chrome/common/zip_reader_unittest.cc:100: ASSERT_TRUE(reader.OpenFromFd(zip_fd)); On 2011/12/13 05:23:08, satorux1 wrote: > We should close the fd, but adding close(fd) here will leak the fd if ASSERT > fails. > > I'd introduce a wrapper class to ensure fd is closed, like > > FdWrapper wrapper(test_zip_file_, FdWrapper::READ_ONLY); > ASSERT_TRUE(reader.OpenFromFd(wrapper.fd()); > Done. http://codereview.chromium.org/8873039/diff/10004/chrome/common/zip_reader_un... chrome/common/zip_reader_unittest.cc:138: int zip_fd = OpenFileReadOnly(test_zip_file_); On 2011/12/13 05:23:08, satorux1 wrote: > ditto. the fd is leaked. Done.
On 2011/12/13 23:14:07, satorux1 wrote: > FWIW, I take back my comment saying "The change will make it impossible to > use the system zlib." > > Looking at > http://src.chromium.org/viewvc/chrome/trunk/src/third_party/zlib/zlib.gyp?vie..., > files under third_party/zlib/contrib are compiled even if use_system_zlib is > specified, so touching files there is actually acceptable, though it'd be nicer > not to touch them anyway. :) I ended up moving everything to zip_internal.cc, it's way cleaner like that.
It's great to see changes under third_party gone! The code looks good. Only style nits. http://codereview.chromium.org/8873039/diff/20001/chrome/common/zip_internal.cc File chrome/common/zip_internal.cc (right): http://codereview.chromium.org/8873039/diff/20001/chrome/common/zip_internal.... chrome/common/zip_internal.cc:71: void* fdopen_file_func(void* opaque, const char* filename, int mode) { Since the code is now in Chrome codebase, the function name should look like FdOpenFileFunc? http://codereview.chromium.org/8873039/diff/20001/chrome/common/zip_internal.... chrome/common/zip_internal.cc:72: FILE* file = NULL; And the indentation is 2, instead of 4. http://codereview.chromium.org/8873039/diff/20001/chrome/common/zip_reader_un... File chrome/common/zip_reader_unittest.cc (right): http://codereview.chromium.org/8873039/diff/20001/chrome/common/zip_reader_un... chrome/common/zip_reader_unittest.cc:34: static const int READ_WRITE = 1; We use enum for this. enum { READ_ONLY, http://codereview.chromium.org/8873039/diff/20001/chrome/common/zip_reader_un... chrome/common/zip_reader_unittest.cc:45: break; we usually have default: in switch. default: NOTREACHED() http://codereview.chromium.org/8873039/diff/20001/chrome/common/zip_reader_un... chrome/common/zip_reader_unittest.cc:122: FdWrapper zip_fd_w(test_zip_file_, FdWrapper::READ_ONLY); zip_fd_wrapper in favor of less abbreviation?
http://codereview.chromium.org/8873039/diff/20001/chrome/common/zip_internal.cc File chrome/common/zip_internal.cc (right): http://codereview.chromium.org/8873039/diff/20001/chrome/common/zip_internal.... chrome/common/zip_internal.cc:71: void* fdopen_file_func(void* opaque, const char* filename, int mode) { On 2011/12/15 00:05:36, satorux1 wrote: > Since the code is now in Chrome codebase, the function name should look like > FdOpenFileFunc? Done. http://codereview.chromium.org/8873039/diff/20001/chrome/common/zip_internal.... chrome/common/zip_internal.cc:72: FILE* file = NULL; On 2011/12/15 00:05:36, satorux1 wrote: > And the indentation is 2, instead of 4. Done. http://codereview.chromium.org/8873039/diff/20001/chrome/common/zip_reader_un... File chrome/common/zip_reader_unittest.cc (right): http://codereview.chromium.org/8873039/diff/20001/chrome/common/zip_reader_un... chrome/common/zip_reader_unittest.cc:34: static const int READ_WRITE = 1; On 2011/12/15 00:05:36, satorux1 wrote: > We use enum for this. > > enum { > READ_ONLY, Done. http://codereview.chromium.org/8873039/diff/20001/chrome/common/zip_reader_un... chrome/common/zip_reader_unittest.cc:45: break; On 2011/12/15 00:05:36, satorux1 wrote: > we usually have default: in switch. > > default: > NOTREACHED() Done. http://codereview.chromium.org/8873039/diff/20001/chrome/common/zip_reader_un... chrome/common/zip_reader_unittest.cc:122: FdWrapper zip_fd_w(test_zip_file_, FdWrapper::READ_ONLY); On 2011/12/15 00:05:36, satorux1 wrote: > zip_fd_wrapper in favor of less abbreviation? Done.
http://codereview.chromium.org/8873039/diff/23001/chrome/common/zip_internal.cc File chrome/common/zip_internal.cc (right): http://codereview.chromium.org/8873039/diff/23001/chrome/common/zip_internal.... chrome/common/zip_internal.cc:71: void* FdOpenFileFunc(void* opaque, const char* filename, int mode) { Function comment is missing. http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi... http://codereview.chromium.org/8873039/diff/23001/chrome/common/zip_internal.... chrome/common/zip_internal.cc:96: void FillFdOpenFileFunc(zlib_filefunc_def* pzlib_filefunc_def, int fd) { function comment. http://codereview.chromium.org/8873039/diff/23001/chrome/common/zip_reader_un... File chrome/common/zip_reader_unittest.cc (right): http://codereview.chromium.org/8873039/diff/23001/chrome/common/zip_reader_un... chrome/common/zip_reader_unittest.cc:34: READ_WRITE }; Sorry to be nit-picky, but enum should be formatted like: enum ReadMode { READ_ONLY, READ_WRITE, }; http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Enumer... http://codereview.chromium.org/8873039/diff/23001/chrome/common/zip_reader_un... chrome/common/zip_reader_unittest.cc:36: FdWrapper(const FilePath& file, int mode) : fd_(-1) { Instead of 'int', please use enum type, like ReadMode. This will make it a bit more type-safe.
http://codereview.chromium.org/8873039/diff/23001/chrome/common/zip_internal.cc File chrome/common/zip_internal.cc (right): http://codereview.chromium.org/8873039/diff/23001/chrome/common/zip_internal.... chrome/common/zip_internal.cc:71: void* FdOpenFileFunc(void* opaque, const char* filename, int mode) { On 2011/12/15 00:40:45, satorux1 wrote: > Function comment is missing. > > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi... Done. http://codereview.chromium.org/8873039/diff/23001/chrome/common/zip_internal.... chrome/common/zip_internal.cc:96: void FillFdOpenFileFunc(zlib_filefunc_def* pzlib_filefunc_def, int fd) { On 2011/12/15 00:40:45, satorux1 wrote: > function comment. Done. http://codereview.chromium.org/8873039/diff/23001/chrome/common/zip_reader_un... File chrome/common/zip_reader_unittest.cc (right): http://codereview.chromium.org/8873039/diff/23001/chrome/common/zip_reader_un... chrome/common/zip_reader_unittest.cc:34: READ_WRITE }; On 2011/12/15 00:40:45, satorux1 wrote: > Sorry to be nit-picky, but enum should be formatted like: > > > enum ReadMode { > READ_ONLY, > READ_WRITE, > }; > > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Enumer... Done. http://codereview.chromium.org/8873039/diff/23001/chrome/common/zip_reader_un... chrome/common/zip_reader_unittest.cc:36: FdWrapper(const FilePath& file, int mode) : fd_(-1) { On 2011/12/15 00:40:45, satorux1 wrote: > Instead of 'int', please use enum type, like ReadMode. This will make it a bit > more type-safe. Done.
LGTM
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jorgelo@chromium.org/8873039/24007
Change committed as 114684 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
