This change is a refactoring cleanup to improve support for compat32
syscalls (and compat64 on CHERI systems). Each process ABI now has it's
own struct sycall instead of using one global list. The list of all
syscalls is replaced with a list of seen syscalls. Looking up the syscall
argument passing convention now interates over the fixed-size array instead
of using a link-list that's populated on startup so we no longer need the
init_syscall() function.
The actual functional changes are in D27625.
Details
Details
Diff Detail
Diff Detail
- Repository
- rS FreeBSD src repository - subversion
- Lint
Lint Passed - Unit
No Test Coverage - Build Status
Buildable 35824 Build 32713: arc lint + arc unit
Event Timeline
| usr.bin/truss/syscall.h | ||
|---|---|---|
| 224 | Personally not a huge fan of syscall_cconv as a name, maybe syscall_decode to match the fact that these come from the decoded_syscalls array? | |
| 225 | I don't think this really belongs in here. You only need it for lookups, so why not make the table an array of struct { char *; struct syscall_cconv; }? | |
| 230 | This belongs in struct syscall IMO (and I don't think you need it here). | |
| usr.bin/truss/syscalls.c | ||
| 1023 | Or just leave it as void as it just returns sc? | |
| 1030 | Hm, might be a little cleaner if there were a separate fixup_syscall, then this could just take a const struct syscall *. It's not obvious that adding a syscall modifies it. | |
| 2984 | Why does this new argument exist? The function only gets called once AFAICT with the value true. It'd probably just be nicer to split out a separate function, or just forego it entirely and note that it's fine because we're about to return from main anyway. | |
| usr.bin/truss/syscall.h | ||
|---|---|---|
| 224 | syscall_decode sounds good to me. | |
| 225 | I wanted to avoid having to change all entries in the table. | |
| usr.bin/truss/syscalls.c | ||
| 1023 | I originally had this function doing the allocation, now the return value is pointless. | |
| 1030 | I could move the fixup to the caller, but then I'd need to do it twice. Also sc can't be const anyway due to the assignment. | |
| usr.bin/truss/syscall.h | ||
|---|---|---|
| 225 | Hm. The reason I don't like it is that it's not a useful field to have for syscalls that don't have special decoding, since it's the ABI-prefixed name (with D27625), not the unprefixed name like some like you'll get for known freebsd32 syscalls, and so if we can instead limit it to only where it's needed (the lookup) then it's consistent and there's no risk of being confused and using the wrong thing. Which also means you can rename syscall's kernel_name back to name, as that's not the best name for it either currently. | |
| usr.bin/truss/syscalls.c | ||
| 1030 | Right you need to deal with the slist in it... I'd still prefer to not have a hidden fixup though. | |
| 1084–1086 | Don't need the variable just check name == NULL. | |
| usr.bin/truss/syscall.h | ||
|---|---|---|
| 225 | I agree it's not useful, but I don't want to edit the entire table. I think changing the table should probably be a separate commit that sorts it to allow use of bsearch. | |
| usr.bin/truss/syscalls.c | ||
| 1030 | The fixup can only be done once we know the ABI, so I think this is probably the right place to do it. | |
| usr.bin/truss/syscall.h | ||
|---|---|---|
| 225 | It'd probably be best to decouple sorting from restructuring, that way it's clear they're NFCs (well, at least git will tell you the sorting was just moving lines). Also neither are really dependent on the other except for the fact that they conflict if done independently. Would you consider leaving kernel_name as name so that a follow-up doesn't need to revert that once the restructuring has happened? | |
| usr.bin/truss/syscalls.c | ||
| 1030 | Well just like you have void add_syscall(struct procabi *abi, u_int number, struct syscall *sc) you'd have void fixup_syscall(struct procabi *abi, struct syscall *sc). | |
| usr.bin/truss/syscalls.c | ||
|---|---|---|
| 1030 | Yes that would also work, but since both will done at the same time we might as well merge them? | |
| usr.bin/truss/setup.c | ||
|---|---|---|
| 484 | I don't understand this change | |
| usr.bin/truss/setup.c | ||
|---|---|---|
| 484 | Yeah I guess that doesn't need to be upstreamed and only makes sense if args is not a long... | |
| usr.bin/truss/main.c | ||
|---|---|---|
| 206 | Sure, happy to drop this. I'm not sure if any allocations have happened though if we hit the error return above. | |
| usr.bin/truss/syscall.h | ||
|---|---|---|
| 226 | It's not the number of return values, but the types. 0 means "doesn't return" (for exit()), 1 means "long" (or int), and 2 means off_t. It could possibly stand to be an enum instead of an int, but not in this commit. | |
| usr.bin/truss/syscalls.c | ||
| 1066 | Why does this have to be allocated? There's no reason to ever free() these things vs just letting exit() reclaim them that I can think of? | |
| usr.bin/truss/syscalls.c | ||
|---|---|---|
| 1068 | This comment probably wants to cover the asprintf vs assigning as non-owned string literal case above too? | |
| usr.bin/truss/syscalls.c | ||
|---|---|---|
| 1062–1074 | I think you probably want this still as style(9) otherwise wants a blank line before the comment, and moving the comment up below the existing blank line seems simpler. | |
| usr.bin/truss/syscalls.c | ||
|---|---|---|
| 1062–1074 | Sorry, missed that suggestion when updating the review. Will fix before committing. | |