[PATCH] D111875: [Support] [Windows] Manually clean up temp files on network shares

[Martin Storsjö via Phabricator via llvm-commits]

mstorsjo added a comment.

In D111875#3071388 <https://reviews.llvm.org/D111875#3071388>, @rnk wrote:

> In D111875#3071350 <https://reviews.llvm.org/D111875#3071350>, @mstorsjo wrote:
>
>> In D111875#3071269 <https://reviews.llvm.org/D111875#3071269>, @rnk wrote:
>>
>>> Here's an idea to try to make this a bit cleaner. Maybe `setDeleteDisposition` should return an error code when the path is on a network drive. That return value would be propagated back up to the caller who sets `OF_Delete`. In this case, that is `TempFile::create`. At that point, TempFile can retry temporary file creation without `OF_Delete`.
>>
>> Hmm... It would indeed be cleaner in the sense that `setDeleteDisposition` doesn't lie about whether it succeeded or not - but that would require using a specific error code for it that the caller would look for (so we don't retry without the flag for any random error) and hope that we don't map other real system errors to that error code.
>
> Yep, it mostly requires making a new error category and defining a new error. A bit annoying, but not too hard. Alternatively, if this is all handled in TempFile::create and no recovery is needed, you can use a generic error code.

Well there's a couple uses in `TempFile::keep()` too. Then again, with a bit more changes, we could stop calling them too, if we aren't using the delete disposition for this file altogether.

>>> The downside to this is that we will create the file, set the disposition (fails), delete the file, and recreate it again without `OF_Delete`.
>>
>> Yeah, that's in practice probably a much bigger overhead than just calling `setDeleteDisposition` one more time.
>>
>>> To improve on that, we could have some kind of `canSetDeleteDisposition` portable API, which would feed into the choice to set the `OF_Delete` flag.
>>
>> Hmm, I guess that could work too. It requires mimicing the logic/checks from `setDeleteDisposition` - or maybe we could remove the logic from there altogether? (I think it should be kept though, so that `OF_Delete` behaves roughly as documented even in those cases, though.)
>
> I think it's already reasonably well-factored already. It all just boils down to calling `is_local` on the directory path where the temporary file will be created, right?

Yes, I guess so.

I guess this sounds like a decent improvement overall - I can try to give this a shot, in a day or two.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D111875/new/

https://reviews.llvm.org/D111875