On Feb 3 12:03, Ben wrote:
> On 26-01-2021 12:34, Corinna Vinschen via Cygwin-patches wrote:
> >> + static bool has_posix_unlink_semantics =
> >> + wincap.has_posix_unlink_semantics ();
> >> + static bool has_posix_unlink_semantics_with_ignore_readonly =
> >> + wincap.has_posix_unlink_semantics_with_ignore_readonly ();
> >
> > Did you mean `const' rather than `static', by any chance? Either way, I
> > don't think these local vars are required, given that the wincap
> > accessors are already marked as const. The compiler should know how to
> > opimize this sufficiently.
> >
> I do mean static.
> With each instantiation these are initialized to the wincap value.
> Then later on, we might disable the behavior if we encounter a driver
> which returns: STATUS_INVALID_PARAMETER
>
> Assuming most files will reside on the same fs, (ie through the same driver)
> this will save use from the system call which we know isn't supported.
But this is an invalid assumption. In fact, it only hold true for
`rm -r' scenarios, not for anything else. What about long running
processes unlinking files in various spots under /var, /tmp, etc.,
i. .e. services. Or what about mc?
Keep in mind that unlink/rmdir are system calls. Each an every call is
independent of each other. If you encounter two unlink calls, you don't
even know how much time has gone by between them. Even if they occur in
a short time frame, you still don't know if they are even remotely
related. For all you know, they could be called from different threads,
doing different things on different partitions.
As such, using a static state at this point and keeping it around for
later calls is a bug.
> >> + ULONG flags = FILE_OPEN_REPARSE_POINT | FILE_OPEN_FOR_BACKUP_INTENT
> >> + | FILE_DELETE_ON_CLOSE | eflags;
> >
> > This looks like a dangerous assumption. So far we don't open unknown
> > reparse points as reparse points deliberately. No one knows what a
> > unknown reparse point is good for or supposed to do, so we don't even
> > know if we are allowed to handle it analogue to a symlink.
> >
> When opening these, you are correct.
> However, when a request is made to delete a reparse point, it's safe
> - even for an unknown reparse point - to assume that it is the reparse point
> itself which is to be deleted. Ofcourse: That's my theory.
It's definitely a deviation from the previous behaviour and I'm not
exactly comfortable with it. The problem is that only a minor part of
the reparse point population is actually something akin to a symlink. I
don't see how it can be a safe bet to allow the user to remove an RP
with unknown and, perhaps, crucial functionality to some given product.
> > Consequentially we open unknown reparse points just as normal files, so
> > that the reparse point's automatisms may kick in. By omitting this
> > step, we're moving on thin ice.
> >
> This would mean an unknown reparse point can never be deleted.
> I'm just not sure if that's what we should want.
It's what we do right now. We're trying to handle all RP types known to
constitute some kind of symlink, and if we learn about the meaning of as
yet unknown RPs, and it *is* some kind of symlink, we can add it to the
list.
> >> +{
> >> + //Step 2
> >> + //Reopen with all sharing flags, will set delete flag ourselves.
> >> + access |= FILE_READ_ATTRIBUTES | FILE_WRITE_ATTRIBUTES;
> >> + flags &= ~FILE_DELETE_ON_CLOSE;
> >> + fstatus = NtOpenFile (&fh, access, attr, &io,
> >> FILE_SHARE_VALID_FLAGS, flags);
> >> + debug_printf ("NtOpenFile %S: %y", attr->ObjectName, fstatus);
> >> +
> >> + if (NT_SUCCESS (fstatus))
> >> +{
> >> + if (has_posix_unlink_semantics_with_ignore_readonly)
> >> +{
> >> + //Step 3
> >> + //Remove the file with POSIX unlink semantics, ignore
> >> readonly flags.
> >
> > No check for NTFS? Posix semantics are not supported on any other FS.
> > No check for remote? Just because you support POSIX semantics on
> > *this* machine, doesn't mean the remote machine supports it at all...
> >
> Indeed no checks.
> If the driver correctly returns STATUS_INVALID_PARAMETER we will not try
> again (by
> resetting the has_posix_unlink_semantics_with_ignore_readonly flag and then
> fallback to
> usual trickery. If the driver returns error (but not
> STATUS_INVALID_PARAMETER) that
> driver pays a single kernel call, which I deem acceptable.
That's back to the static var then, which isn't feasible. For non-NTFS
or remote FSes you will introduce a constant penalty.
> >> + //As we don't have posix unlink semantics, this
> >> will still fail if the file is in use.
> >
> > Without transaction?
> >
> Well, yes, the transaction overhead doesn't weigh up to the unlikeliness of
> failure, I think.
The transaction would only be called for DOS R/O files anyway, which is
a minor part of the file population with a pret
