[PATCH] D81803: [Support] PR42623: Avoid setting the delete-on-close bit if a TempFile doesn't reside on a local drive

[Ronald Wampler via Phabricator via llvm-commits]

rdwampler added inline comments.


================
Comment at: llvm/lib/Support/Windows/Path.inc:407
+  // set DeleteFile to true, since it prevents opening the file for writes.
+  SmallVector<wchar_t, 128> FinalPath;
+  if (std::error_code EC = realPathFromHandle(Handle, FinalPath))
----------------
jhenderson wrote:
> aganea wrote:
> > The error occurs on a second call with `setDeleteDisposition(..., false)` initiated by `TempFile::keep()`. A simple repro from https://bugs.llvm.org/show_bug.cgi?id=48378#c0 can be found here: https://pastebin.com/v51m3uBU
> > 
> > We could enclose this new block of code into `if (Delete) { ... }`. eg.
> > ```
> > static std::error_code setDeleteDisposition(HANDLE Handle, bool Delete) {
> >   if (Delete) {
> >     // First, check if the file is on a network (non-local) drive. If so, don't
> >     // set DeleteFile to true, since it prevents opening the file for writes.
> >     SmallVector<wchar_t, 128> FinalPath;
> >     ...
> >     // At this point, the file is on a local drive.
> >   }
> > 
> >   FILE_DISPOSITION_INFO Disposition;
> >   Disposition.DeleteFile = Delete;
> >   if (!SetFileInformationByHandle(Handle, FileDispositionInfo, &Disposition,
> >                                   sizeof(Disposition)))
> >     return mapWindowsError(::GetLastError());
> >   return std::error_code();
> > }
> > 
> > ```
> > The original report from @rdwampler suggests that the problem is not `SetFileInformationByHandle`, but writes that happen later in the process. In this particular case, the writes already happened.
> > 
> > WDYT?
> Not sure who the WDYT is directed at here, but I'm happy for alternative approaches that solve the same issue.
I'm leaning towards reverting this change and letting the callers handle this logic. Currently, the only caller is `TempFile`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81803