Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1303343007:
Created a blocking copy to temporary file function. (Closed)
|
Created:
5 years, 3 months ago by Sean Klein Modified:
5 years, 3 months ago CC:
mojo-reviews_chromium.org Base URL:
https://github.com/domokit/mojo.git@master Target Ref:
refs/heads/master Project:
mojo Visibility:
Public. |
DescriptionCreated a blocking copy to temporary file function.
This functionality will be useful for creating temporary nexe files
from the URLResponse in the nexe content handler.
This change also modifies the SFI and nonsfi content handlers to use this
new function.
BUG=https://github.com/domokit/mojo/issues/396
R=mseaborn@chromium.org
Committed: https://chromium.googlesource.com/external/mojo/+/a2b469511861b02b29a1b4c22805df467dcd420b
Patch Set 1 #
Total comments: 5
Patch Set 2 : #Patch Set 3 : Added new BlockingCopy function to SFI content handler #
Total comments: 12
Patch Set 4 : Switched to ScopedFILE, formatted #
Total comments: 5
Patch Set 5 : Responded to code review #
Total comments: 4
Patch Set 6 : Less NULL, more nullptr #
Total comments: 6
Patch Set 7 : #
Messages
Total messages: 23 (3 generated)
smklein@chromium.org changed reviewers: + jamesr@chromium.org, mseaborn@chromium.org
Please add this code in the nexe content handler. When it's needed in multiple different places we can move it into a common location but not before.
On 2015/09/01 23:27:41, jamesr wrote: > Please add this code in the nexe content handler. When it's needed in multiple > different places we can move it into a common location but not before. One may object that BlockingCopyHelper is internal, but probably the thing to do is to just use base::CreateTemporaryFile() and then use BlockingCopyToFile() and then open/unlink. (One may also object that this would be inefficient, but then base's temporary file functions are already inefficient. One may also object that this greatly widens the window during which the file may be "leaked", which is true. Arguably, BlockingCopyToFile() should take a FILE* rather than a path.)
viettrungluu@chromium.org changed reviewers: + viettrungluu@chromium.org
https://codereview.chromium.org/1303343007/diff/1/mojo/data_pipe_utils/data_p... File mojo/data_pipe_utils/data_pipe_file_utils.cc (right): https://codereview.chromium.org/1303343007/diff/1/mojo/data_pipe_utils/data_p... mojo/data_pipe_utils/data_pipe_file_utils.cc:333: FILE** fpp) { Possibly this should be a base::ScopedFILE*. Or you could return a base::ScopedFILE, with it being null on failure. https://codereview.chromium.org/1303343007/diff/1/mojo/data_pipe_utils/data_p... mojo/data_pipe_utils/data_pipe_file_utils.cc:342: LOG(ERROR) << "Failed to unlink temporary file\n"; Also: no \n needed here. More importantly, it's perhaps a bit unexpected that *fpp gets set even on failure here.
So I'm a bit confused -- are you suggesting I move this functionality to the nexe content handler, but modify BlockingCopyToFile such that it can take a FILE* rather than a path, and use that instead of "BlockingCopyHelper" in the nexe handling code? Also -- our main reason for creating this function is to avoid the "open --> copy --> unlink" workflow, and use "open --> unlink --> copy" instead.
https://codereview.chromium.org/1303343007/diff/1/mojo/data_pipe_utils/data_p... File mojo/data_pipe_utils/data_pipe_file_utils.cc (right): https://codereview.chromium.org/1303343007/diff/1/mojo/data_pipe_utils/data_p... mojo/data_pipe_utils/data_pipe_file_utils.cc:333: FILE** fpp) { On 2015/09/01 23:36:25, viettrungluu wrote: > Possibly this should be a base::ScopedFILE*. > > Or you could return a base::ScopedFILE, with it being null on failure. Out of curiosity, what is the difference between a ScopedFILE and a FILE? I see the documentation in base which says "Automatically closes |FILE*|s", but it is not clear to me when this ScopedFILE is actually closed. Would it be when no one holds a reference to the FILE * anymore? https://codereview.chromium.org/1303343007/diff/1/mojo/data_pipe_utils/data_p... mojo/data_pipe_utils/data_pipe_file_utils.cc:342: LOG(ERROR) << "Failed to unlink temporary file\n"; On 2015/09/01 23:36:25, viettrungluu wrote: > Also: no \n needed here. > More importantly, it's perhaps a bit unexpected that *fpp gets set even on > failure here. Done.
https://codereview.chromium.org/1303343007/diff/1/mojo/data_pipe_utils/data_p... File mojo/data_pipe_utils/data_pipe_file_utils.cc (right): https://codereview.chromium.org/1303343007/diff/1/mojo/data_pipe_utils/data_p... mojo/data_pipe_utils/data_pipe_file_utils.cc:333: FILE** fpp) { On 2015/09/01 at 23:57:52, smklein1 wrote: > On 2015/09/01 23:36:25, viettrungluu wrote: > > Possibly this should be a base::ScopedFILE*. > > > > Or you could return a base::ScopedFILE, with it being null on failure. > > Out of curiosity, what is the difference between a ScopedFILE and a FILE? I see the documentation in base which says "Automatically closes |FILE*|s", but it is not clear to me when this ScopedFILE is actually closed. Would it be when no one holds a reference to the FILE * anymore? base::ScopedFILE's destructor calls fclose()
Patchset #3 (id:40001) has been deleted
In response to Mark's request: "Sean, can you change services/nacl/content_handler_main.cc to use the new function? It's better to avoid adding functions that aren't used anywhere (either from tests or other code). Then you could actually remove BlockingCopyToFile() since it's not used anywhere else (and it's not in a "public" dir)." This is done.
https://codereview.chromium.org/1303343007/diff/60001/mojo/data_pipe_utils/da... File mojo/data_pipe_utils/data_pipe_file_utils.cc (right): https://codereview.chromium.org/1303343007/diff/60001/mojo/data_pipe_utils/da... mojo/data_pipe_utils/data_pipe_file_utils.cc:331: base::Bind(&CopyToFileHelper, fp))) { Nit: fix indentation https://codereview.chromium.org/1303343007/diff/60001/mojo/data_pipe_utils/da... mojo/data_pipe_utils/data_pipe_file_utils.cc:332: LOG(ERROR) << "Could not copy source to temporary file"; Nit: this error path doesn't fclose(fp). You could either call fclose(fp) or used ScopedFILE. https://codereview.chromium.org/1303343007/diff/60001/services/nacl/content_h... File services/nacl/content_handler_main.cc (right): https://codereview.chromium.org/1303343007/diff/60001/services/nacl/content_h... services/nacl/content_handler_main.cc:27: const GURL& url) { Nit: Fix indentation. Fits on previous line? You could try using "git cl format" for this. https://codereview.chromium.org/1303343007/diff/60001/services/nacl/content_h... services/nacl/content_handler_main.cc:83: if ((irt_fp_ = TempFileForURL(url_loader, irt_url)) == NULL) { I'd prefer using two lines for this, to avoid doing assignments inside conditionals. (I'm not sure if the style guide takes a position on this.) https://codereview.chromium.org/1303343007/diff/60001/services/nacl/content_h... services/nacl/content_handler_main.cc:114: int irt_fd = fileno(irt_fp_); It looks like you could combine: fileno() + dup() + NaClDescCreateWithFilePathMetadata() + fclose() (+ maybe BlockingCopyToTempFile()) into one function so that this sequence isn't duplicated between the nexe and IRT cases. https://codereview.chromium.org/1303343007/diff/60001/services/nacl/content_h... services/nacl/content_handler_main.cc:144: if (fclose(irt_fp_)) { Nit: you could do this before LaunchNaCl()
https://codereview.chromium.org/1303343007/diff/60001/mojo/data_pipe_utils/da... File mojo/data_pipe_utils/data_pipe_file_utils.cc (right): https://codereview.chromium.org/1303343007/diff/60001/mojo/data_pipe_utils/da... mojo/data_pipe_utils/data_pipe_file_utils.cc:331: base::Bind(&CopyToFileHelper, fp))) { On 2015/09/02 18:46:45, Mark Seaborn wrote: > Nit: fix indentation Done. https://codereview.chromium.org/1303343007/diff/60001/mojo/data_pipe_utils/da... mojo/data_pipe_utils/data_pipe_file_utils.cc:332: LOG(ERROR) << "Could not copy source to temporary file"; On 2015/09/02 18:46:45, Mark Seaborn wrote: > Nit: this error path doesn't fclose(fp). You could either call fclose(fp) or > used ScopedFILE. Using ScopedFILE. https://codereview.chromium.org/1303343007/diff/60001/services/nacl/content_h... File services/nacl/content_handler_main.cc (right): https://codereview.chromium.org/1303343007/diff/60001/services/nacl/content_h... services/nacl/content_handler_main.cc:27: const GURL& url) { On 2015/09/02 18:46:45, Mark Seaborn wrote: > Nit: Fix indentation. Fits on previous line? > > You could try using "git cl format" for this. Done. https://codereview.chromium.org/1303343007/diff/60001/services/nacl/content_h... services/nacl/content_handler_main.cc:83: if ((irt_fp_ = TempFileForURL(url_loader, irt_url)) == NULL) { On 2015/09/02 18:46:45, Mark Seaborn wrote: > I'd prefer using two lines for this, to avoid doing assignments inside > conditionals. > > (I'm not sure if the style guide takes a position on this.) Done. https://codereview.chromium.org/1303343007/diff/60001/services/nacl/content_h... services/nacl/content_handler_main.cc:114: int irt_fd = fileno(irt_fp_); On 2015/09/02 18:46:45, Mark Seaborn wrote: > It looks like you could combine: > > fileno() + dup() + NaClDescCreateWithFilePathMetadata() + fclose() > (+ maybe BlockingCopyToTempFile()) > > into one function so that this sequence isn't duplicated between the nexe and > IRT cases. Done. https://codereview.chromium.org/1303343007/diff/60001/services/nacl/content_h... services/nacl/content_handler_main.cc:144: if (fclose(irt_fp_)) { On 2015/09/02 18:46:45, Mark Seaborn wrote: > Nit: you could do this before LaunchNaCl() Done. https://codereview.chromium.org/1303343007/diff/80001/services/nacl/content_h... File services/nacl/content_handler_main.cc (right): https://codereview.chromium.org/1303343007/diff/80001/services/nacl/content_h... services/nacl/content_handler_main.cc:72: if (fclose(file_stream)) { Is this still necessary with a ScopedFILE?
LGTM https://codereview.chromium.org/1303343007/diff/80001/services/nacl/content_h... File services/nacl/content_handler_main.cc (right): https://codereview.chromium.org/1303343007/diff/80001/services/nacl/content_h... services/nacl/content_handler_main.cc:72: if (fclose(file_stream)) { On 2015/09/02 21:49:11, smklein1 wrote: > Is this still necessary with a ScopedFILE? No. This fclose() currently isn't right because this is still owned by the ScopedFILE. (It happens that you currently don't get a double-free() because your ScopedFILEs are scoped such that NaClExit() is called before their destructors can run.) One fix is to remove this fclose() call. A better fix is to change get() to release() below... https://codereview.chromium.org/1303343007/diff/80001/services/nacl/content_h... services/nacl/content_handler_main.cc:140: NaClDesc* irt_desc = FileStreamToNaClDesc(irt_fp_.get()); I think you can use release() instead of get() for these two calls to transfer ownership of the FILE*.
I had to modify content_handler_main_nonsfi to get it to build, since it was added by https://codereview.chromium.org/1323823002/ https://codereview.chromium.org/1303343007/diff/80001/services/nacl/content_h... File services/nacl/content_handler_main.cc (right): https://codereview.chromium.org/1303343007/diff/80001/services/nacl/content_h... services/nacl/content_handler_main.cc:72: if (fclose(file_stream)) { On 2015/09/02 22:50:35, Mark Seaborn wrote: > On 2015/09/02 21:49:11, smklein1 wrote: > > Is this still necessary with a ScopedFILE? > > No. This fclose() currently isn't right because this is still owned by the > ScopedFILE. > > (It happens that you currently don't get a double-free() because your > ScopedFILEs are scoped such that NaClExit() is called before their destructors > can run.) > > One fix is to remove this fclose() call. A better fix is to change get() to > release() below... Done. https://codereview.chromium.org/1303343007/diff/80001/services/nacl/content_h... services/nacl/content_handler_main.cc:140: NaClDesc* irt_desc = FileStreamToNaClDesc(irt_fp_.get()); On 2015/09/02 22:50:35, Mark Seaborn wrote: > I think you can use release() instead of get() for these two calls to transfer > ownership of the FILE*. Done.
Few drive-bys https://codereview.chromium.org/1303343007/diff/100001/mojo/data_pipe_utils/d... File mojo/data_pipe_utils/data_pipe_file_utils.cc (right): https://codereview.chromium.org/1303343007/diff/100001/mojo/data_pipe_utils/d... mojo/data_pipe_utils/data_pipe_file_utils.cc:324: return NULL; s/NULL/nullptr/. Only use NULL in code that has to compile as C https://codereview.chromium.org/1303343007/diff/100001/mojo/data_pipe_utils/d... mojo/data_pipe_utils/data_pipe_file_utils.cc:333: return NULL; ditto
https://codereview.chromium.org/1303343007/diff/100001/mojo/data_pipe_utils/d... File mojo/data_pipe_utils/data_pipe_file_utils.cc (right): https://codereview.chromium.org/1303343007/diff/100001/mojo/data_pipe_utils/d... mojo/data_pipe_utils/data_pipe_file_utils.cc:324: return NULL; On 2015/09/02 23:29:31, jamesr wrote: > s/NULL/nullptr/. Only use NULL in code that has to compile as C Whoops! Sorry, a bit new to C++, and old habits die hard apparently. https://codereview.chromium.org/1303343007/diff/100001/mojo/data_pipe_utils/d... mojo/data_pipe_utils/data_pipe_file_utils.cc:333: return NULL; On 2015/09/02 23:29:31, jamesr wrote: > ditto Done.
I just noticed that this review didn't get CC'd to mojo-reviews. That is supposed to happen automatically. Any idea why it didn't? LGTM for the change. https://codereview.chromium.org/1303343007/diff/120001/services/nacl/content_... File services/nacl/content_handler_main.cc (right): https://codereview.chromium.org/1303343007/diff/120001/services/nacl/content_... services/nacl/content_handler_main.cc:110: if (irt_fp_ == nullptr) { "!irt_fp_" is the normal style for Chromium code. https://codereview.chromium.org/1303343007/diff/120001/services/nacl/content_... services/nacl/content_handler_main.cc:132: if (nexe_fp == nullptr) { Ditto: "!nexe_fp" https://codereview.chromium.org/1303343007/diff/120001/services/nacl/content_... File services/nacl/content_handler_main_nonsfi.cc (right): https://codereview.chromium.org/1303343007/diff/120001/services/nacl/content_... services/nacl/content_handler_main_nonsfi.cc:46: if (nexe_fp == nullptr) { Ditto
https://codereview.chromium.org/1303343007/diff/120001/services/nacl/content_... File services/nacl/content_handler_main.cc (right): https://codereview.chromium.org/1303343007/diff/120001/services/nacl/content_... services/nacl/content_handler_main.cc:110: if (irt_fp_ == nullptr) { On 2015/09/03 06:33:16, Mark Seaborn wrote: > "!irt_fp_" is the normal style for Chromium code. Done. https://codereview.chromium.org/1303343007/diff/120001/services/nacl/content_... services/nacl/content_handler_main.cc:132: if (nexe_fp == nullptr) { On 2015/09/03 06:33:16, Mark Seaborn wrote: > Ditto: "!nexe_fp" Done. https://codereview.chromium.org/1303343007/diff/120001/services/nacl/content_... File services/nacl/content_handler_main_nonsfi.cc (right): https://codereview.chromium.org/1303343007/diff/120001/services/nacl/content_... services/nacl/content_handler_main_nonsfi.cc:46: if (nexe_fp == nullptr) { On 2015/09/03 06:33:16, Mark Seaborn wrote: > Ditto Done.
Message was sent while issue was closed.
Committed patchset #7 (id:140001) manually as a2b469511861b02b29a1b4c22805df467dcd420b (presubmit successful).
Message was sent while issue was closed.
On 2015/09/03 at 06:33:16, mseaborn wrote: > I just noticed that this review didn't get CC'd to mojo-reviews. That is supposed to happen automatically. Any idea why it didn't? Sean - how did you upload this patch initially? mojo-reviews@chromium.org is added by the CC_LIST of the mojo repo's codereview.settings which should be parsed by 'git cl upload'.
Message was sent while issue was closed.
On 2015/09/03 16:49:51, jamesr wrote: > On 2015/09/03 at 06:33:16, mseaborn wrote: > > I just noticed that this review didn't get CC'd to mojo-reviews. That is > supposed to happen automatically. Any idea why it didn't? > > Sean - how did you upload this patch initially? mailto:mojo-reviews@chromium.org is > added by the CC_LIST of the mojo repo's codereview.settings which should be > parsed by 'git cl upload'. I uploaded this patch the same as any of my other mojo commits. I think I might have accidentally removed mojo-reviews from the CC list partway through responding to the code reviews, when I was trying to prevent spamming anyone other than the people on the reviewers list. I won't modify the default CC list for future Cls. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
