Introduce funlinkat.

[jilles]

sys/kern/vfs_syscalls.c

1790

Both EINTR and ERESTART would be wrong here, since once such an error has been returned by a sleep with PCATCH, any sleep with PCATCH will immediately return that error until the signal has been handled. Therefore such errors need to be returned to the caller.

The restart in unlinking needs a different return value, possibly not an error number.

[kib]

sys/kern/vfs_syscalls.c

1790

This is exactly what I tried to note.

[cem]

This is a good idea, but please incorporate kib/jilles feedback. asomers' manual page suggestion is not a bad one either, although it can possibly be just an addition to unlinkat.

[oshogbo]

I did change the implementation to match the unlinkat.
I add man page.

[jilles]

lib/libc/sys/unlink.2

47

I would prefer a different order that keeps the directory fd and path together.

115–119

This seems to mean to say:

except in the case where
.Fa fd
is not \-1.

It may be worth it to change this -1 to another value to improve error detection, just like FD_ATCWD is not -1 in most implementations.

sys/kern/capabilities.conf

465

The new system call is named fdunlinkat elsewhere in the patch. Also, removing unlinkat here breaks binary compatibility.

sys/kern/syscalls.master

1030

The audit identifier AUE_OPENAT_RWTC is definitely wrong. The simple option is AUE_UNLINKAT; alternatively, add a new one that includes the extra fd.

[kib]

sys/kern/capabilities.conf

465

This permit should be added to sys/compat/freebsd32/capabilities.conf as well.

sys/kern/vfs_syscalls.c

1800

Same comment about setting fp to NULL and checking fp != NULL instead of fd != -1 on fdout.

1814

I think

if (error == EINVAL)
  error = EPERM;

is cleaner.

1824

Same as for fdrmdirat, use fp->f_vnode directly there.

3732

Set fp to NULL there.

3767

You only use vfdp there, IMO it would be cleaner to do

if (fp != NULL && fp->f_vnode != vp)

3800

if (fp != NULL)
   fdrop(fp, td);

[oshogbo]

• At FD_NONE as suggested by @jilles
• Fix compat32
• Simplify code like suggested by @kib
• Fix master.syscall
• Fix manapge as suggested by @jilles
• Change the api as suggested by @jilles
• Set fd cap righs to CAP_LOOKUP

[kib]

lib/libc/sys/unlink.2

119

Too many except's ? It is not clear if second 'except' is sub-except of the first or not.

123

'are corresponding' sounds weird and unprecise. You might say 'point to the same file' e.g.

sys/kern/vfs_syscalls.c

1870

Unneeded {}.

[asomers]

lib/libc/sys/unlink.2

117

This is grammatically incorrect English. Nor is "name" defined as an argument. It should be "path". I suggest

except in the case where the file specified by
.Fa path
is already open as the file descriptor
.Fa fd .

120

What happens if those arguments are not corresponding? The "equivalent to" descriptor suggests that in this case, the path will still be removed. But that's not what actually happens. You should probably amend the description to say that fdunlinkat is equivalent to unlinkat "in the case where" (note lack of "except"). Then add that "otherwise the path will not be removed and an error will be returned. But that still leaves the reader in a state of mystery, wondering why this syscall exists at all. An explanatory statement would help. So something like this:

The
.Fn fdunlinkat
system call can be used to unlink an already-opened file, unless that  file has been replaced since it was opened.
It is equivalent to
.Fn unlinkat
in the case where
.Fa name
is already open as the file descriptor
.Fa fd .
Otherwise, the path will not be removed and an error will be returned.

220

You can remove this "the".

sys/kern/syscalls.master

1030

Why do you reuse AUE_OPENAT_RWTC?

[oshogbo]

I used sentance proposed by @asomers but also add some information about FD_NONE.
Fix style proposed by @kib

[kib]

I do not have any important notes about this change except the error code when raced.

lib/libc/sys/unlink.2

232

I find this error code to be not specific enough for the case. I believe that callers of this syscall would be interested to know that the file was renamed before the attempt to remove.

sys/kern/vfs_syscalls.c

1749

if ((flag & ~AT_REMOVEDIR) != 0)

1752

Same.

[oshogbo]

lib/libc/sys/unlink.2

232

Do you have any particular errno on your main?
Now I thinking about:

35 EAGAIN Resource temporarily unavailable.

[kib]

lib/libc/sys/unlink.2

232

I do not have preference, it should be an error logically impossible with the normal delete. We assign unusual meaning to it anway. EGAIN, EDEADLK, ESPIPE all look good enough.

[oshogbo]

Change error code to the EAGAIN

[jilles]

lib/libc/sys/unlink.2

232

Reusing EAGAIN here has the problem that in other EAGAIN cases there is a reasonable possibility that the same call will succeed later (for example, after fork() failed and other processes have terminated or after recv() failed and the other side sent some data). Although fdunlinkat() may succeed after having failed because of a mismatch (for example, if the file was renamed back), this feels like a contrived situation.

If you want to repurpose an existing error, EDEADLK, ESPIPE or EMLINK look more reasonable, but still not very nice. A new errno may be more appropriate.

sys/sys/fcntl.h

323

Please make the numerical value different from AT_FDCWD to catch mixing these up.

[kib]

lib/libc/sys/unlink.2

232

Changing size of sys_errlist is a mess. I do not think that it is worth to induce it for such reason.

[jilles]

I think this looks good except that I would like to consider a new errno again.

lib/libc/sys/unlink.2

232

I don't think introducing new errnos should be impossible; apart from this one, ETIME should really be a unique error and not an alias for ETIMEDOUT.

Regarding sys_errlist, perhaps we should make this such that dynamically linked applications directly accessing sys_errlist will be limited to the current 97 elements or 6.0's 93 elements (exporting a separate sys_nerr for them) and stop worrying about adding new errors.

[kib]

lib/libc/sys/unlink.2

232

ETIME would simplify porting the linux kernel code, indeed. Introducing new error codes is sure possible, with the consequences and significant amount of work.

I do not feel that introducing the error code for fdunlinkat() race failure worth the efforts. Unix API has very strong tradition of overloading error codes. I do not mean EINVAL fail, but other EXXX usage is mostly frivolous and does not cause objections when used not for the situation directly described by the errstr.

I do not object strongly for addition if this would be done by somebody else, but myself I would certainly decided to go with the existing number.

[oshogbo]

@cem, @kib, @emaste, @jilles I would like to commit it as-it-is but pjd suggested to change the name to funlinkat? After some thoughts I also think that funlinkat is a bether name 'fd' suggests that it's working on 'FILE *'. Do you agree?

[brooks]

lib/libc/sys/Symbol.map

403

Do we need a new symbol version number for 13 or do you definitely plan to MFC this before 12.0?

sys/kern/syscalls.master

1030

This part of the diff is very out of date. Please make sure to follow the new style in this file.

[brooks]

In D14567#383658, @oshogbo wrote:

@cem, @kib, @emaste, @jilles I would like to commit it as-it-is but pjd suggested to change the name to funlinkat? After some thoughts I also think that funlinkat is a bether name 'fd' suggests that it's working on 'FILE *'. Do you agree?

funlinkat it certainly more idiomatic. There are many syscalls with f*() names that act on file descriptors and none which match fd*() (ignoring fdatasync which matches by accident.)

[asomers]

In D14567#383658, @oshogbo wrote:

@cem, @kib, @emaste, @jilles I would like to commit it as-it-is but pjd suggested to change the name to funlinkat? After some thoughts I also think that funlinkat is a bether name 'fd' suggests that it's working on 'FILE *'. Do you agree?

I agree with pjd. We already have about 19 syscalls starting with "f" that take file descriptor arguments and none starting with "fd" (except fdatasync, of course). And there are no few library functions starting with "fd" that take file descriptors but not FILE* pointers (except fdevname, of course).

[kib]

lib/libc/sys/Symbol.map

403

Sure, this cannot be committed as is. You need FBSD_1.6 namespace for the new symbol, regardless of the intent of merging it to 12.0 (which should be done as well).

[cem]

In D14567#383658, @oshogbo wrote:

@cem, @kib, @emaste, @jilles I would like to commit it as-it-is but pjd suggested to change the name to funlinkat? After some thoughts I also think that funlinkat is a bether name 'fd' suggests that it's working on 'FILE *'. Do you agree?

+1 to funlinkat() name.

[oshogbo]

Update patch.

[asomers]

lib/libc/sys/unlink.2

134

You should start a new sentence for "In that case".

220

You still need to remove the "the" before .Fn unlinkat.

283

It should be 12.1 by now.

[oshogbo]

Address @asomers suggestions.

[oshogbo]

Address @asomers suggestions.

[asomers]

LGTM. But you should still probably get a final review from the likes of @jilles or @kib .

[kib]

sys/kern/vfs_syscalls.c

1840

There is a corner situation possible which might deserve more delicate handling. Since fp->f_vnode is not locked while the reference is held, it is possible for the f_vnode to be reclaimed, and then namei() call above would allocate the new vnode for the same inode. So despite user specified correct file descriptor, the condition is not true and we return EDEADLK.

IMO, you should check the VI_DOOMED flag and return EBADF instead of EDEADLK if f_vnode != vp and VI_DOOMED and set.

3796

Same note about VI_DOOMED there.

[oshogbo]

Address @kib suggestion.

[kib]

I would write it in single-line

error = (fp->f_vnode->v_iflag & VI_DOOMED) != 0 ? EBADF : EDEADLK;