Page MenuHomeFreeBSD

Introduce funlinkat.
ClosedPublic

Authored by oshogbo on Mar 2 2018, 6:25 PM.

Diff Detail

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
jilles added inline comments.Mar 2 2018, 10:08 PM
sys/kern/vfs_syscalls.c
1790 ↗(On Diff #39889)

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 added inline comments.Mar 2 2018, 10:21 PM
sys/kern/vfs_syscalls.c
1790 ↗(On Diff #39889)

This is exactly what I tried to note.

cem added a reviewer: cem.Mar 3 2018, 6:18 PM
cem requested changes to this revision.Mar 24 2018, 5:18 PM

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.

This revision now requires changes to proceed.Mar 24 2018, 5:18 PM
oshogbo updated this revision to Diff 43502.Jun 9 2018, 5:32 PM
oshogbo retitled this revision from Introduce unlinkfd. to Introduce fdunlinkat..

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

oshogbo marked 6 inline comments as done.Jun 9 2018, 5:33 PM
jilles requested changes to this revision.Jun 10 2018, 9:14 AM
jilles added inline comments.
lib/libc/sys/unlink.2
47 ↗(On Diff #43502)

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

115–119 ↗(On Diff #43502)

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 ↗(On Diff #43502)

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

sys/kern/syscalls.master
1030 ↗(On Diff #43502)

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.

This revision now requires changes to proceed.Jun 10 2018, 9:14 AM
kib added inline comments.Jun 10 2018, 9:48 AM
sys/kern/capabilities.conf
465 ↗(On Diff #43502)

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

sys/kern/vfs_syscalls.c
1800 ↗(On Diff #43502)

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

1814 ↗(On Diff #43502)

I think

if (error == EINVAL)
  error = EPERM;

is cleaner.

1824 ↗(On Diff #43502)

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

3732 ↗(On Diff #43502)

Set fp to NULL there.

3767 ↗(On Diff #43502)

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

if (fp != NULL && fp->f_vnode != vp)
3800 ↗(On Diff #43502)
if (fp != NULL)
   fdrop(fp, td);
oshogbo updated this revision to Diff 43548.Jun 10 2018, 3:40 PM
oshogbo marked 10 inline comments as done.
  • 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 accepted this revision.Jun 10 2018, 4:16 PM
kib added inline comments.
lib/libc/sys/unlink.2
119 ↗(On Diff #43548)

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

123 ↗(On Diff #43548)

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

sys/kern/vfs_syscalls.c
1870 ↗(On Diff #43548)

Unneeded {}.

asomers added inline comments.Jun 10 2018, 4:45 PM
lib/libc/sys/unlink.2
117 ↗(On Diff #43502)

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 ↗(On Diff #43502)

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 ↗(On Diff #43502)

You can remove this "the".

sys/kern/syscalls.master
1030 ↗(On Diff #39889)

Why do you reuse AUE_OPENAT_RWTC?

oshogbo updated this revision to Diff 43620.Jun 11 2018, 9:06 PM
oshogbo marked an inline comment as done.

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

oshogbo marked 6 inline comments as done.Jun 11 2018, 9:06 PM
kib added a comment.Jun 12 2018, 11:31 AM

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

lib/libc/sys/unlink.2
232 ↗(On Diff #43620)

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 ↗(On Diff #43620)

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

1752 ↗(On Diff #43620)

Same.

oshogbo updated this revision to Diff 44035.Jun 18 2018, 8:45 PM
oshogbo marked 2 inline comments as done.
oshogbo added inline comments.
lib/libc/sys/unlink.2
232 ↗(On Diff #43620)

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

35 EAGAIN Resource temporarily unavailable.
kib added inline comments.Jun 18 2018, 9:00 PM
lib/libc/sys/unlink.2
232 ↗(On Diff #43620)

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 updated this revision to Diff 44045.Jun 18 2018, 9:37 PM

Change error code to the EAGAIN

jilles added inline comments.Jun 21 2018, 9:26 PM
lib/libc/sys/unlink.2
232 ↗(On Diff #43620)

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 ↗(On Diff #44045)

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

kib added inline comments.Jun 21 2018, 9:36 PM
lib/libc/sys/unlink.2
232 ↗(On Diff #43620)

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

oshogbo updated this revision to Diff 44516.Jun 27 2018, 10:44 AM
oshogbo marked an inline comment as done.
oshogbo marked 4 inline comments as done.
kib accepted this revision.Jun 27 2018, 12:10 PM

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

lib/libc/sys/unlink.2
232 ↗(On Diff #43620)

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 added inline comments.Jul 4 2018, 2:17 PM
lib/libc/sys/unlink.2
232 ↗(On Diff #43620)

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.

@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 added inline comments.Nov 12 2018, 6:10 PM
lib/libc/sys/Symbol.map
403 ↗(On Diff #44516)

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 ↗(On Diff #44516)

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

@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.)

@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 added inline comments.Nov 12 2018, 6:28 PM
lib/libc/sys/Symbol.map
403 ↗(On Diff #44516)

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 added a comment.Nov 12 2018, 9:30 PM

@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 updated this revision to Diff 52946.Jan 17 2019, 9:15 PM
oshogbo retitled this revision from Introduce fdunlinkat. to Introduce funlinkat..

Update patch.

asomers requested changes to this revision.Jan 17 2019, 9:28 PM
asomers added inline comments.
lib/libc/sys/unlink.2
134 ↗(On Diff #52946)

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

283 ↗(On Diff #52946)

It should be 12.1 by now.

220 ↗(On Diff #43502)

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

This revision now requires changes to proceed.Jan 17 2019, 9:28 PM
oshogbo updated this revision to Diff 52947.Jan 17 2019, 9:40 PM

Address @asomers suggestions.

oshogbo updated this revision to Diff 52949.Jan 17 2019, 9:42 PM
oshogbo marked 2 inline comments as done.

Address @asomers suggestions.

asomers accepted this revision.Jan 17 2019, 9:44 PM

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

kib added inline comments.Jan 19 2019, 12:58 PM
sys/kern/vfs_syscalls.c
1840 ↗(On Diff #52949)

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 ↗(On Diff #52949)

Same note about VI_DOOMED there.

oshogbo updated this revision to Diff 53040.Jan 20 2019, 5:24 PM

Address @kib suggestion.

oshogbo updated this revision to Diff 53248.Jan 26 2019, 1:12 PM
kib accepted this revision.Jan 26 2019, 5:12 PM

I would write it in single-line

error = (fp->f_vnode->v_iflag & VI_DOOMED) != 0 ? EBADF : EDEADLK;
This revision was not accepted when it landed; it landed in state Needs Review.Apr 6 2019, 9:34 AM
This revision was automatically updated to reflect the committed changes.