ino64
Needs ReviewPublic

Authored by kib on Thu, Apr 20, 5:55 PM.

Details

Reviewers
mckusick
emaste
Group Reviewers
manpages
Summary

See post-patch /testing.txt for explanation.

Diff Detail

Repository
rS FreeBSD src repository
Lint
Lint Skipped
Unit
Unit Tests Skipped
Build Status
Buildable 8816
Build 9187: CI src buildJenkins
kib created this revision.Thu, Apr 20, 5:55 PM
ngie added a subscriber: ngie.Fri, Apr 21, 3:21 AM

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

This probably shouldn't be committed.

abi.sh
1

Hmmm... this is an interesting script. What does it do?

contrib/compiler-rt/lib/sanitizer_common/sanitizer_linux.cc
226–227

Should the "hacks" be upstreamed? They seem more preferable over calling lstat, stat, etc.

contrib/openbsm/libbsm/bsm_wrappers.c
267

Would rwatson care about these changes?

lib/libc/gen/devname-compat11.c
43

Why int instead of size_t?

lib/libc/gen/fts-compat.c
746

dirp != NULL?

lib/libc/gen/fts-compat11.c
124

Please don't use nitems (it masks the nitems(..) macro in sys/param.h).

182

compar != NULL?

216

Unnecessary spaces between O_RDONLY, |, and O_CLOEXEC?

243

cp[1] != '\0'

263

.. != NULL

275

The if-statement is unnecessary per-POSIX.

311

style(9): char *t comes before int instr?

638

Move this down to sort per style(9)?

650

Remove the #ifdef since this feature has been present since 1997?

$ git blame include/fts.h | grep FTS_WHITEOUT
53b81178e2d0a (peter 1997-03-11 11:11:37 +0000 57) #define FTS_WHITEOUT 0x080 /* return whiteout information */

651–654
oflag = DTF_NODUP | DTR_REWIND;
if (ISSET(FTS_WHITEOUT))
    oflag |= DTR_HIDEW;
```?
686

Remove deadcode?

796

DT_DIR has been around since 1994 -- this should probably be removed too:

$ git blame sys/sys/dirent.h | grep DT_DIR
8fb65ce818b3e (rgrimes 1994-05-24 10:09:53 +0000 70) #define DT_DIR 4

882–883

style(9): sort by type?

1109

#undef ADJUST?

1151

oerrno vs saved_errno is a bit of an odd convention.

lib/libc/gen/fts-compat11.h
33

_FTS_COMPAT11_H?

45

fts_compare?

lib/libc/gen/ftw-compat11.c
38

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

sverrno vs saved_errno vs oerrno is inconsistent.

lib/libc/gen/getmntinfo-compat11.c
51

style(9): sort values?

60–64

Could this use reallocf or reallocarray instead?

lib/libc/gen/glob-compat11.c
189

Move the memset out in both scenarios to reduce duplication?

229

Whitespace issue here and the line after next?

939–940

style(9): sort?

1021

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

SCANDIR_B?

lib/libc/sys/getdirentries.2
35

Bump .Dd at commit time?

lib/libc/sys/lstat.c
33–34

Sort sys/stat.h before sys/syscall.h?

lib/libc/sys/mknod.c
34

Sort sys/stat.h before sys/syscall.h?

lib/libc/sys/stat.c
32

Why sys/fcntl.h instead of fcntl.h? fcntl.h is more portable.

34

Sort sys/stat.h before sys/syscall.h?

lib/libc/sys/statfs.2
70

Should this be bumped to a more recent date?

lib/libprocstat/libprocstat_compat.c
33–34

Sort headers?

share/man/man5/dir.5
109–114

This seems like it could be split off of this CR.

Also: should MAXNAMLEN be wrapped if a #ifndef MAXNAMLEN?

136

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

Why COMPAT_43?

1898

#ifdef?

sys/compat/linux/linux_file.c
1093–1094

This should arguably be a different constant because Linux might have different values from FreeBSD...

sys/fs/nfsserver/nfs_nfsdport.c
2029–2039

I'm confused. Why is the VFS_VGET only being done for zfs now?

sys/kern/kern_descrip.c
1307

#ifdef?

sys/kern/vfs_syscalls.c
646–647

Could this be committed separately?

3944

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

Why XXXKIB?

sys/sys/dirent.h
119–122

Is this a bit of a rehash of other headers?

sys/sys/mount.h
69

Bump the version number?

testing.txt
1

Remove file?

usr.bin/kdump/kdump.c
935

Why remove this? Should it be committed separately?

emaste added inline comments.
TODO.ino64
1

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

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

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
lib/libc/gen/devname-compat11.c
43

because that's the type in FreeBSD 11's devname_r.

lib/libc/gen/fts-compat.c
746

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

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

Although it's really just an opaque value, might as well use a recent (commit?) date.

sys/nlm/nlm_advlock.c
221

In the style of XXXRW etc.

testing.txt
1

Yes this one is also not for inclusion in the commit

kib added a comment.Fri, Apr 21, 2:11 PM

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

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

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.

gleb added a subscriber: gleb.Fri, Apr 21, 4:33 PM