Page MenuHomeFreeBSD

Introduce funlinkat.
ClosedPublic

Authored by oshogbo on Mar 2 2018, 6:25 PM.
Tags
None
Referenced Files
Unknown Object (File)
Sun, Mar 17, 3:17 AM
Unknown Object (File)
Sun, Mar 17, 12:52 AM
Unknown Object (File)
Sat, Mar 16, 10:49 PM
Unknown Object (File)
Fri, Mar 8, 11:49 PM
Unknown Object (File)
Fri, Mar 8, 11:49 PM
Unknown Object (File)
Fri, Mar 8, 11:49 PM
Unknown Object (File)
Fri, Mar 8, 10:57 PM
Unknown Object (File)
Fri, Mar 8, 10:57 PM

Diff Detail

Lint
Lint Skipped
Unit
Tests Skipped

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
sys/kern/vfs_syscalls.c
1813

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.

sys/kern/vfs_syscalls.c
1813

This is exactly what I tried to note.

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 retitled this revision from Introduce unlinkfd. to Introduce fdunlinkat..

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

jilles requested changes to this revision.Jun 10 2018, 9:14 AM
jilles added inline comments.
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–466

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.

This revision now requires changes to proceed.Jun 10 2018, 9:14 AM
sys/kern/capabilities.conf
465–466

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

sys/kern/vfs_syscalls.c
1801

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.

1825

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

3734

Set fp to NULL there.

3768

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

if (fp != NULL && fp->f_vnode != vp)
3801
if (fp != NULL)
   fdrop(fp, td);
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 added inline comments.
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 {}.

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.
229

You can remove this "the".

sys/kern/syscalls.master
1030

Why do you reuse AUE_OPENAT_RWTC?

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

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 marked 2 inline comments as done.
oshogbo added inline comments.
lib/libc/sys/unlink.2
232

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

35 EAGAIN Resource temporarily unavailable.
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.

Change error code to the EAGAIN

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.

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.

oshogbo marked an inline comment as done.
oshogbo marked 4 inline comments as done.

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.

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.

@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?

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.

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

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, @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 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
126

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

229

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

257

It should be 12.1 by now.

This revision now requires changes to proceed.Jan 17 2019, 9:28 PM
oshogbo marked 2 inline comments as done.

Address @asomers suggestions.

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

sys/kern/vfs_syscalls.c
1825

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.

3768

Same note about VI_DOOMED there.

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.