NEW 183288
offlineasm: Use temporary file for generating assembly code 
https://bugs.webkit.org/show_bug.cgi?id=183288
Summary offlineasm: Use temporary file for generating assembly code
Dominik Inführ
Reported 2018-03-02 05:28:18 PST
offlineasm: Use temporary file for generating assembly code
Attachments
Patch (1.77 KB, patch)
2018-03-02 05:37 PST, Dominik Inführ 		no flags
DetailsFormatted DiffDiff
Patch (2.03 KB, patch)
2018-03-04 23:52 PST, Dominik Inführ 		no flags
DetailsFormatted DiffDiff
Patch (1.63 KB, patch)
2018-12-14 00:21 PST, Dominik Inführ 		no flags
DetailsFormatted DiffDiff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
Dominik Inführ
Comment 1 2018-03-02 05:37:19 PST
Created attachment 334897 [details] Patch
JF Bastien
Comment 2 2018-03-02 09:13:59 PST
Comment on attachment 334897 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=334897&action=review One suggestion, otherwise r=me > Source/JavaScriptCore/offlineasm/asm.rb:398 > +FileUtils.cp(tempFile, outputFlnm) I think you want rename, not copy: http://ruby-doc.org/core-2.1.1/File.html#method-c-rename If it follows UNIX convention then it'll be atomic.
Dominik Inführ
Comment 3 2018-03-04 23:52:25 PST
Created attachment 334988 [details] Patch
Dominik Inführ
Comment 4 2018-03-04 23:55:21 PST
Thanks for the review and the suggestion. I've updated the patch to use File.rename but fall back to FileUtils.cp if the rename should fail. This happens to be the case on my system, where /tmp is on another file system.
JF Bastien
Comment 5 2018-03-05 09:51:42 PST
(In reply to Dominik Inführ from comment #4) > Thanks for the review and the suggestion. I've updated the patch to use > File.rename but fall back to FileUtils.cp if the rename should fail. This > happens to be the case on my system, where /tmp is on another file system. Hmm interesting... Your backup isn't atomic then, and you're trying to remove potentially truncated files. Maybe instead, when it fails, you should cp to a tempname in the same directory (so it'll be the same mount point), and then rename that. That would keep the actual file atomic. WDYT? r=me if you think that's overkill, but it should be simple.
Dominik Inführ
Comment 6 2018-12-14 00:21:30 PST
Created attachment 357306 [details] Patch
Dominik Inführ
Comment 7 2018-12-14 00:24:18 PST
Could you please take a look again? I am now always generating the temporary file in the same directory of the output file. Then rename'ing should work and Tempfile isn't necessary anymore.
Alex Christensen
Comment 8 2021-11-01 12:43:59 PDT
Comment on attachment 357306 [details] Patch This has been requesting review for more than one year. If this is still needed, please rebase and re-request review.
Note You need to log in before you can comment on or make changes to this bug.