ino64
ClosedPublic

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
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
There are a very large number of changes, so older changes are hidden. Show Older Changes
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
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
53b81178e2d0a (peter 1997-03-11 11:11:37 +0000 57) #define FTS_WHITEOUT 0x080 /* return whiteout information */

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
8fb65ce818b3e (rgrimes 1994-05-24 10:09:53 +0000 70) #define DT_DIR 4

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?

emaste added inline comments.
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

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
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.

gleb added a subscriber: gleb.Apr 21 2017, 4:33 PM
wblock added a subscriber: wblock.May 5 2017, 3:29 PM
wblock added inline comments.
sys/sys/dirent.h
55 ↗(On Diff #27581)

s/a 8/an 8/

testing.txt
53 ↗(On Diff #27581)

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.May 5 2017, 3:41 PM
kib added inline comments.
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.

wblock added inline comments.May 5 2017, 4:35 PM
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.
kib marked an inline comment as done.May 5 2017, 5:27 PM
kib updated this revision to Diff 28105.May 6 2017, 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.May 18 2017, 3:22 PM

Commit candidate.

This revision was automatically updated to reflect the committed changes.
wblock added inline comments.Jul 7 2017, 6:40 PM
testing.txt
53 ↗(On Diff #27581)

s/depended/depending/