[PATCH] Add ability for can_write to indicate why the file is not writable

[Aaron Ballman]

On Thu, Sep 4, 2014 at 7:54 PM, Richard Smith <richard at metafoo.co.uk> wrote:
> On Thu, Sep 4, 2014 at 10:47 AM, David Blaikie <dblaikie at gmail.com> wrote:
>>
>> On Thu, Sep 4, 2014 at 4:49 AM, Aaron Ballman <aaron at aaronballman.com>
>> wrote:
>>>
>>> On Wed, Sep 3, 2014 at 9:36 PM, Richard Smith <richard at metafoo.co.uk>
>>> wrote:
>>> > Hi dblaikie,
>>> >
>>> > 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"`).
>>> >
>>> > 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:
>>> >
>>> >   std::error_code can_write(const Twine &Path, bool &result);
>>> >
>>> > ... which seems redundant because `result` is set to `true` iff the
>>> > error code is `success`, and ...
>>> >
>>> >   std::error_code can_write(const Twine &Path);
>>> >
>>> > ... which is terrible because `bool(can_write(Path))` is `true` iff the
>>> > file is *not* writable, and ...
>>> >
>>> >   std::error_code cannot_write(const Twine &Path);
>>> >
>>> > ... which is inconsistent with other functions near here and ugly.
>>>
>>> I don't think this API (in general) is a good one to have in the first
>>> place as it suffers from TOCTOU issues (as do our uses of other APIs
>>> like fs::exists). A better approach is for places like
>>> CompilerInstance.cpp is to simply open the file for writing and report
>>> the various ways that can fail instead of asking questions for which
>>> 1) the answer can change subsequent to asking the question anyway, and
>>> 2) the answer isn't canonical (there's a lot more reasons for not
>>> being able to write to a file than its "read-only" attribute being set
>>> on Windows, for instance).
>>
>>
>> +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.
>
>
> 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.

Does the work we save turn into actual performance gains we care about?

> 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.
>
> The code is approximately:
>
>   if (exists(dest) && !can_write(dest)) {
>     fill in error code;
>     return;
>   }
>
>   produce_temporary_file();
>   if (!copy_temporary_file_to_dest()) {
>     fill in error code;
>     return;
>   }
>
> ... and the problem is that we can't fill in the error code in the first
> case.

What do we lose by getting rid of that first case entirely, because
the second case already has to handle the exact same problems
regardless? Eg) File exists and can be written to, passing your first
checks, but when we attempt to produce/copy the temp file and that
fails because the file has been deleted or been flagged as not
writable.

If the work we save is considerable, then I am okay with the idea
behind this patch so long as the actual attempt to open the file for
writing handles failure gracefully too.

~Aaron