ino64
Needs ReviewPublic

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

Details

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 9360
kib created this revision.Apr 20 2017, 5:55 PM
ngie added a subscriber: ngie.Apr 21 2017, 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
2

This probably shouldn't be committed.

abi.sh
2

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

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

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
44

Why int instead of size_t?

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

dirp != NULL?

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

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

183

compar != NULL?

217

Unnecessary spaces between O_RDONLY, |, and O_CLOEXEC?

244

cp[1] != '\0'

264

.. != NULL

276

The if-statement is unnecessary per-POSIX.

312

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

639

Move this down to sort per style(9)?

651

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 */

652–655
oflag = DTF_NODUP | DTR_REWIND;
if (ISSET(FTS_WHITEOUT))
    oflag |= DTR_HIDEW;
```?
687

Remove deadcode?

797

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

883–884

style(9): sort by type?

1110

#undef ADJUST?

1152

oerrno vs saved_errno is a bit of an odd convention.

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

_FTS_COMPAT11_H?

46

fts_compare?

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

This should be maxfds to match the manpage. opengroup.org calls it ndirs though, so maybe that should be the code standard of sorts.

91

sverrno vs saved_errno vs oerrno is inconsistent.

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

style(9): sort values?

61–65

Could this use reallocf or reallocarray instead?

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

Move the memset out in both scenarios to reduce duplication?

230

Whitespace issue here and the line after next?

940–941

style(9): sort?

1022

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
56

SCANDIR_B?

lib/libc/sys/getdirentries.2
35

Bump .Dd at commit time?

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

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

lib/libc/sys/mknod.c
35

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

lib/libc/sys/stat.c
33

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

35

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
34–35

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
1869

Why COMPAT_43?

1906

#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
2031–2042

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
605–607

Could this be committed separately?

3719

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–123

Is this a bit of a rehash of other headers?

sys/sys/mount.h
69

Bump the version number?

testing.txt
2

Remove file?

usr.bin/kdump/kdump.c
935

Why remove this? Should it be committed separately?

emaste added inline comments.
TODO.ino64
2

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
2

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–228

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
44

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
46

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
2

Yes this one is also not for inclusion in the commit

kib added a comment.Apr 21 2017, 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
2

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–228

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.Apr 21 2017, 4:33 PM
wblock added a subscriber: wblock.Fri, May 5, 3:29 PM
wblock added inline comments.
sys/sys/dirent.h
55

s/a 8/an 8/

testing.txt
54

s/depended/dependant/
or possibly "depending". The "in general" at the end is kind of confusing.

We handled kinfo sysctl MIBs, but, in general, other MIBs which report structures
depending on the changed type are not handled.
kib marked an inline comment as done.Fri, May 5, 3:41 PM
kib added inline comments.
testing.txt
54

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.

wblock added inline comments.Fri, May 5, 4:35 PM
testing.txt
54

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.
kib marked an inline comment as done.Fri, May 5, 5:27 PM
kib updated this revision to Diff 28105.Sat, May 6, 12:06 PM

Update to the current patch. Most important change is probably a space to extend time_t in struct stat on i386.

kib updated this revision to Diff 28523.Thu, May 18, 3:22 PM

Commit candidate.