Page MenuHomeFreeBSD

Always set td_errno to the error value of a system call.
ClosedPublic

Authored by jhb on Jul 9 2019, 11:03 PM.

Details

Summary

Early errors prior to a system call did not set td_errno. This commit
sets td_errno for all errors during syscallenter(). As a result,
syscallret() can now always use td_errno without checking TDP_NERRNO.

Diff Detail

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

Event Timeline

sys/kern/kern_fork.c
1086 ↗(On Diff #59584)

For safety (?), I would also clear td_errno in thread_create().

sys/kern/subr_syscall.c
88 ↗(On Diff #59584)

if (error != 0), since you are changing the line anyway.

124 ↗(On Diff #59584)

I would write td->rd_errno = error = ECAPMODE;

175 ↗(On Diff #59584)

Do we need the error argument anymore ?

201 ↗(On Diff #59584)

I would left this line as is, just in case.

sys/kern/subr_syscall.c
175 ↗(On Diff #59584)

We do not, though that makes for a larger patch that touches all the architectures. I could do that as a followup so that if for some reason someone needs to bisect it is more isolated to this change than the mechanical one to remove the arg. It might be that we no longer need to return a value from syscallenter() either. I haven't looked yet to see what the fallout is from removing the arg to syscallret().

201 ↗(On Diff #59584)

The reason I took it out is that syscallret doesn't check this flag anymore, so I felt it was cleaner to only clear it in syscallenter() since that is now the only function that checks it.

sys/kern/subr_syscall.c
175 ↗(On Diff #59584)

A followup cleanup is fine. Perhaps you want to mark the error arg with __unused in this commit ?

201 ↗(On Diff #59584)

I was mostly concerned with somebody examining the td_pflags in some other thread life moment.

jhb marked 2 inline comments as done.Jul 10 2019, 4:19 PM
jhb added inline comments.
sys/kern/kern_fork.c
1086 ↗(On Diff #59584)

I could do it in thead_ctor() perhaps, or in HEAD I could just move td_errno up into the zero section. For MFC I'd have to set it explicitly. If I do that I can axe setting it here.

jhb marked an inline comment as done.Jul 10 2019, 4:41 PM
jhb added inline comments.
sys/kern/kern_fork.c
1086 ↗(On Diff #59584)

FYI, the reason I set it here was that I was trying to be sure that td_errno was always set to a valid value when TDB_SCX was set and grepping for TDB_SCX is how I ended up at this function.

  • Rebase.
  • Style fixes from kib.
  • Move td_errno into zeroed section of struct thread.
This revision is now accepted and ready to land.Jul 10 2019, 6:22 PM