<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Thu, Sep 4, 2014 at 10:47 AM, David Blaikie <span dir="ltr"><<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div><div class="h5">On Thu, Sep 4, 2014 at 4:49 AM, Aaron Ballman <span dir="ltr"><<a href="mailto:aaron@aaronballman.com" target="_blank">aaron@aaronballman.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div>On Wed, Sep 3, 2014 at 9:36 PM, Richard Smith <<a href="mailto:richard@metafoo.co.uk" target="_blank">richard@metafoo.co.uk</a>> wrote:<br>

> Hi dblaikie,<br>
><br>
> Add an optional output parameter to `can_write` to allow it to indicate why a file is not considered writable (along with a sample use from Clang where we currently produce stupid diagnostics saying `"can't open output file: Success"`).<br>

><br>
> I'm not entirely sure this is the right design, and in particular it's a somewhat different interface from the other functions in this file. I also considered:<br>
><br>
>   std::error_code can_write(const Twine &Path, bool &result);<br>
><br>
> ... which seems redundant because `result` is set to `true` iff the error code is `success`, and ...<br>
><br>
>   std::error_code can_write(const Twine &Path);<br>
><br>
> ... which is terrible because `bool(can_write(Path))` is `true` iff the file is *not* writable, and ...<br>
><br>
>   std::error_code cannot_write(const Twine &Path);<br>
><br>
> ... which is inconsistent with other functions near here and ugly.<br>
<br>
</div>I don't think this API (in general) is a good one to have in the first<br>
place as it suffers from TOCTOU issues (as do our uses of other APIs<br>
like fs::exists). A better approach is for places like<br>
CompilerInstance.cpp is to simply open the file for writing and report<br>
the various ways that can fail instead of asking questions for which<br>
1) the answer can change subsequent to asking the question anyway, and<br>
2) the answer isn't canonical (there's a lot more reasons for not<br>
being able to write to a file than its "read-only" attribute being set<br>
on Windows, for instance).<br></blockquote><div><br></div></div></div><div>+1 to all of that. Chandler & others have tales of TOCTOU stuff that scares me sufficiently to assume that's all applicable & worth worrying about, but perhaps Richard has a notion as to why it might not apply here? Otherwise I'm all for making this a "try to do the thing you want (open for write, etc) and see if it fails" rather than querying that property separately.</div></div></div></div></blockquote><div><br></div><div>There's no TOCTOU issue here. We're merely doing an early check to see if a later operation is likely to fail so we can save some work and exit earlier. We can't "try to do the thing we want" because the thing we want is a rename, and we've not built the source file yet.</div><div><br></div><div>The code is approximately:</div><div><br></div><div>  if (exists(dest) && !can_write(dest)) {</div><div>    fill in error code;</div><div>    return;</div><div>  }</div><div>  </div><div>  produce_temporary_file();</div><div>  if (!copy_temporary_file_to_dest()) {</div><div>    fill in error code;</div><div>    return;</div><div>  }</div><div><br></div><div>... and the problem is that we can't fill in the error code in the first case.</div></div></div></div>