Bug 183288 - offlineasm: Use temporary file for generating assembly code
Summary: offlineasm: Use temporary file for generating assembly code
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2018-03-02 05:28 PST by Dominik Inführ
Modified: 2021-11-01 12:43 PDT (History)
10 users (show)

See Also:


Attachments
Patch (1.77 KB, patch)
2018-03-02 05:37 PST, Dominik Inführ
no flags Details | Formatted Diff | Diff
Patch (2.03 KB, patch)
2018-03-04 23:52 PST, Dominik Inführ
no flags Details | Formatted Diff | Diff
Patch (1.63 KB, patch)
2018-12-14 00:21 PST, Dominik Inführ
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dominik Inführ 2018-03-02 05:28:18 PST
offlineasm: Use temporary file for generating assembly code
Comment 1 Dominik Inführ 2018-03-02 05:37:19 PST
Created attachment 334897 [details]
Patch
Comment 2 JF Bastien 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.
Comment 3 Dominik Inführ 2018-03-04 23:52:25 PST
Created attachment 334988 [details]
Patch
Comment 4 Dominik Inführ 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.
Comment 5 JF Bastien 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.
Comment 6 Dominik Inführ 2018-12-14 00:21:30 PST
Created attachment 357306 [details]
Patch
Comment 7 Dominik Inführ 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.
Comment 8 Alex Christensen 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.