See post-patch /testing.txt for explanation.
Details
Details
- Reviewers
mckusick emaste - Group Reviewers
manpages - Commits
- rS318736: Commit the 64-bit inode project.
Diff Detail
Diff Detail
- Repository
- rS FreeBSD src repository - subversion
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
There are a very large number of changes, so older changes are hidden. Show Older Changes
Comment Actions
Thank you very much for working on this.
I took a very quick pass over some spots. I understand that some of this is copying old compat code, but I was wondering if now was the right time to fix stylistic issues and other nits before duplicating more copies of the same code around.
| TODO.ino64 | ||
|---|---|---|
| 1 ↗ | (On Diff #27581) | This probably shouldn't be committed. |
| abi.sh | ||
| 1 ↗ | (On Diff #27581) | Hmmm... this is an interesting script. What does it do? |
| contrib/compiler-rt/lib/sanitizer_common/sanitizer_linux.cc | ||
| 226–227 ↗ | (On Diff #27581) | Should the "hacks" be upstreamed? They seem more preferable over calling lstat, stat, etc. |
| contrib/openbsm/libbsm/bsm_wrappers.c | ||
| 267 ↗ | (On Diff #27581) | Would rwatson care about these changes? |
| lib/libc/gen/devname-compat11.c | ||
| 43 ↗ | (On Diff #27581) | Why int instead of size_t? |
| lib/libc/gen/fts-compat.c | ||
| 746 ↗ | (On Diff #27581) | dirp != NULL? |
| lib/libc/gen/fts-compat11.c | ||
| 124 ↗ | (On Diff #27581) | Please don't use nitems (it masks the nitems(..) macro in sys/param.h). |
| 182 ↗ | (On Diff #27581) | compar != NULL? |
| 216 ↗ | (On Diff #27581) | Unnecessary spaces between O_RDONLY, |, and O_CLOEXEC? |
| 243 ↗ | (On Diff #27581) | cp[1] != '\0' |
| 263 ↗ | (On Diff #27581) | .. != NULL |
| 275 ↗ | (On Diff #27581) | The if-statement is unnecessary per-POSIX. |
| 311 ↗ | (On Diff #27581) | style(9): char *t comes before int instr? |
| 638 ↗ | (On Diff #27581) | Move this down to sort per style(9)? |
| 650 ↗ | (On Diff #27581) | Remove the #ifdef since this feature has been present since 1997? $ git blame include/fts.h | grep FTS_WHITEOUT |
| 651–654 ↗ | (On Diff #27581) | oflag = DTF_NODUP | DTR_REWIND;
if (ISSET(FTS_WHITEOUT))
oflag |= DTR_HIDEW;
```? |
| 686 ↗ | (On Diff #27581) | Remove deadcode? |
| 796 ↗ | (On Diff #27581) | DT_DIR has been around since 1994 -- this should probably be removed too: $ git blame sys/sys/dirent.h | grep DT_DIR |
| 882–883 ↗ | (On Diff #27581) | style(9): sort by type? |
| 1109 ↗ | (On Diff #27581) | #undef ADJUST? |
| 1151 ↗ | (On Diff #27581) | oerrno vs saved_errno is a bit of an odd convention. |
| lib/libc/gen/fts-compat11.h | ||
| 33 ↗ | (On Diff #27581) | _FTS_COMPAT11_H? |
| 45 ↗ | (On Diff #27581) | fts_compare? |
| lib/libc/gen/ftw-compat11.c | ||
| 38 ↗ | (On Diff #27581) | This should be maxfds to match the manpage. opengroup.org calls it ndirs though, so maybe that should be the code standard of sorts. |
| 90 ↗ | (On Diff #27581) | sverrno vs saved_errno vs oerrno is inconsistent. |
| lib/libc/gen/getmntinfo-compat11.c | ||
| 51 ↗ | (On Diff #27581) | style(9): sort values? |
| 60–64 ↗ | (On Diff #27581) | Could this use reallocf or reallocarray instead? |
| lib/libc/gen/glob-compat11.c | ||
| 189 ↗ | (On Diff #27581) | Move the memset out in both scenarios to reduce duplication? |
| 229 ↗ | (On Diff #27581) | Whitespace issue here and the line after next? |
| 939–940 ↗ | (On Diff #27581) | style(9): sort? |
| 1021 ↗ | (On Diff #27581) | I've seen this sprinkled around this file quite a bit. It would be nice if there was a better reference to the comment of interest since initial comment # 2 is vague. |
| lib/libc/gen/scandir-compat11.c | ||
| 55 ↗ | (On Diff #27581) | SCANDIR_B? |
| lib/libc/sys/getdirentries.2 | ||
| 35 ↗ | (On Diff #27581) | Bump .Dd at commit time? |
| lib/libc/sys/lstat.c | ||
| 33–34 ↗ | (On Diff #27581) | Sort sys/stat.h before sys/syscall.h? |
| lib/libc/sys/mknod.c | ||
| 34 ↗ | (On Diff #27581) | Sort sys/stat.h before sys/syscall.h? |
| lib/libc/sys/stat.c | ||
| 32 ↗ | (On Diff #27581) | Why sys/fcntl.h instead of fcntl.h? fcntl.h is more portable. |
| 34 ↗ | (On Diff #27581) | Sort sys/stat.h before sys/syscall.h? |
| lib/libc/sys/statfs.2 | ||
| 70 ↗ | (On Diff #27581) | Should this be bumped to a more recent date? |
| lib/libprocstat/libprocstat_compat.c | ||
| 33–34 ↗ | (On Diff #27581) | Sort headers? |
| share/man/man5/dir.5 | ||
| 109–114 ↗ | (On Diff #27581) | This seems like it could be split off of this CR. Also: should MAXNAMLEN be wrapped if a #ifndef MAXNAMLEN? |
| 136 ↗ | (On Diff #27581) | Could this be split off of the CR and committed separately (well, not the dp/namlen part, but the rest)? |
| sys/compat/freebsd32/freebsd32_misc.c | ||
| 1861 ↗ | (On Diff #27581) | Why COMPAT_43? |
| 1898 ↗ | (On Diff #27581) | #ifdef? |
| sys/compat/linux/linux_file.c | ||
| 1093–1094 ↗ | (On Diff #27581) | This should arguably be a different constant because Linux might have different values from FreeBSD... |
| sys/fs/nfsserver/nfs_nfsdport.c | ||
| 2029–2039 ↗ | (On Diff #27581) | I'm confused. Why is the VFS_VGET only being done for zfs now? |
| sys/kern/kern_descrip.c | ||
| 1307 ↗ | (On Diff #27581) | #ifdef? |
| sys/kern/vfs_syscalls.c | ||
| 646–647 ↗ | (On Diff #27581) | Could this be committed separately? |
| 3944 ↗ | (On Diff #27581) | Could this be tunable for folks who have potentially large directories (maybe not in the compat syscall, but maybe in the non-compat one)? |
| sys/nlm/nlm_advlock.c | ||
| 221 ↗ | (On Diff #27581) | Why XXXKIB? |
| sys/sys/dirent.h | ||
| 119–122 ↗ | (On Diff #27581) | Is this a bit of a rehash of other headers? |
| sys/sys/mount.h | ||
| 69 ↗ | (On Diff #27581) | Bump the version number? |
| testing.txt | ||
| 1 ↗ | (On Diff #27581) | Remove file? |
| usr.bin/kdump/kdump.c | ||
| 935 ↗ | (On Diff #27581) | Why remove this? Should it be committed separately? |
| TODO.ino64 | ||
|---|---|---|
| 1 ↗ | (On Diff #27581) | Indeed; this Phabricator review is a snapshot of the diff to the GitHub branch https://github.com/FreeBSDFoundation/freebsd/tree/ino64, including some text files which won't be committed. |
| abi.sh | ||
| 1 ↗ | (On Diff #27581) | This is a wrapper around abi-compliance-checker https://www.freshports.org/devel/abi-compliance-checker/ to compare ABIs of the upstream & ino64 branches. It could be made more general and placed in tools/ but for now it's a one-off for the project branch. |
| contrib/compiler-rt/lib/sanitizer_common/sanitizer_linux.cc | ||
| 226–227 ↗ | (On Diff #27581) | fstatat has been around since FreeBSD 8 so there should be no trouble using it universally and upstreaming this. @dim? |
| contrib/openbsm/libbsm/bsm_wrappers.c | ||
| 267 ↗ | (On Diff #27581) | |
| lib/libc/gen/devname-compat11.c | ||
| 43 ↗ | (On Diff #27581) | because that's the type in FreeBSD 11's devname_r. |
| lib/libc/gen/fts-compat.c | ||
| 746 ↗ | (On Diff #27581) | yeah, style(9) bug exists in old version but if the line is going to be changed anyway might as well address it |
| lib/libc/gen/fts-compat11.h | ||
| 45 ↗ | (On Diff #27581) | in the great UNIX tradition of leaving off trailing es this has been spelled fts_compar since the beginning of FreeBSD. |
| lib/libc/sys/statfs.2 | ||
| 70 ↗ | (On Diff #27581) | Although it's really just an opaque value, might as well use a recent (commit?) date. |
| sys/nlm/nlm_advlock.c | ||
| 221 ↗ | (On Diff #27581) | In the style of XXXRW etc. |
| testing.txt | ||
| 1 ↗ | (On Diff #27581) | Yes this one is also not for inclusion in the commit |
Comment Actions
I deliberately refuse to make any gratuitous style changes in the copied files. They have to be kept in sync with the master copies.
I did not found a sane way to avoid the copying and not make the non-compat code the ugly mess. If anybody have sensible ideas how to achieve that, I am more than happy to implement the proposal.
| TODO.ino64 | ||
|---|---|---|
| 1 ↗ | (On Diff #27581) | If you really looked at the files, esp. at the preapply.sh, you would see how all this is handled. |
| contrib/compiler-rt/lib/sanitizer_common/sanitizer_linux.cc | ||
| 226–227 ↗ | (On Diff #27581) | It is not about 'preferable'. The stat(2) syscall is removed in the patch, otherwise I would not touch llvm code with a long pole. |
| sys/sys/dirent.h | ||
|---|---|---|
| 55 ↗ | (On Diff #27581) | s/a 8/an 8/ |
| testing.txt | ||
| 53 ↗ | (On Diff #27581) | s/depended/dependant/ We handled kinfo sysctl MIBs, but, in general, other MIBs which report structures depending on the changed type are not handled. |
| testing.txt | ||
|---|---|---|
| 53 ↗ | (On Diff #27581) | The word meant to inform that we provide no general mechanism to handle sysctls which return structures with changed layout. Moving 'in general' to the place you proposed changes the meaning. |
| testing.txt | ||
|---|---|---|
| 53 ↗ | (On Diff #27581) | Ah, that helps. Well, see what you think: We handled kinfo sysctl MIBs, but there is no general mechanism to handle other sysctl MIBS which return structures where the layout has changed. |
Comment Actions
Update to the current patch. Most important change is probably a space to extend time_t in struct stat on i386.
| testing.txt | ||
|---|---|---|
| 53 ↗ | (On Diff #27581) | s/depended/depending/ |