Date: Tue, 18 Jan 2011 06:14:47 +0200 From: Gleb Kurtsou <gleb.kurtsou@gmail.com> To: Garrett Cooper <yanegomi@gmail.com> Cc: fs@freebsd.org Subject: Re: Inconsistency and bug with *chflags functions Message-ID: <20110118041447.GA2087@tops.skynet.lt> In-Reply-To: <AANLkTintj_eM=aRF4pO8PcT8VXvVJ=Booxag%2BwBg3gaX@mail.gmail.com> References: <AANLkTintj_eM=aRF4pO8PcT8VXvVJ=Booxag%2BwBg3gaX@mail.gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
On (17/01/2011 16:32), Garrett Cooper wrote: > Hi FS folks, > > Originally I did this to determine why lchflags had an int > argument and chflags/fchflags had unsigned long arguments, and I ran > into an interesting mini-project (by accident) after I found a few > bugs lurking in the vfs_syscalls.c layer of the kernel. > > So here's the deal... > > Problems: > 1. chflags/fchflags claim that the flags argument is an unsigned long, > whereas lchflags claims it's an int. This is inconsistent. > 2. The kernel interfaces are all implicitly downcasting the flags > argument to int. > 3. There's a sizing/signedness discrepancy in a few other structures > with fixed-width data types (uint32_t). > > Solution: > 1. I opted to convert fflags_t to uint32_t and convert all references > which did direct conversions to and from st_flags to fflags_t to avoid > 32-bit / 64-bit biarch issues. I downgraded it to uint32_t as we're no > where near the limit for the number of usable flags to *chflags(2). > 2. *chflags now uses fflags_t instead of int/unsigned long. > 3. c_file_flags in dumprestore.h was changed to be in sync with st_flags. > 4. di_flags in ufs/ufs/dinode.h was changed to be in sync with st_flags. > > Compatibility: > 1. NetBSD uses unsigned long in their chflags calls (both in kernel > and userland) so they're more consistent than we are by not having > mixed flags calling convention like us, but uses uint32_t in their > data structures (like we do), so they have a 32-bit/64-bit biarch bug > (again like we do). > 2. OpenBSD is using unsigned int, so I assume that their kernel layer > is also using unsigned int (I am basing this purely off the manpage as > I haven't looked at their sources, but I could be wrong). Hi Garrett, You've changed syscalls definitions but didn't add backward compatibility shims for kernel, libc and freebsd32. I'm working on changing ino_t to 64 bits and nlink_t to 32 bits, we could join the effort, as projects look related. My old patch is available here: https://github.com/downloads/glk/freebsd-ino64/freebsd-ino64-patch.tgz Please find comments to the patch below. > > I'm running make universe just to see if it will barf in the > build, but would someone please review this change (I made the change > as minimal as possible for ease of review) and provide feedback? > Thanks, > -Garrett > > PS Please CC me on all replies as I'm not subscribed to the list. > Index: include/protocols/dumprestore.h > =================================================================== > --- include/protocols/dumprestore.h (revision 217362) > +++ include/protocols/dumprestore.h (working copy) > @@ -95,7 +95,7 @@ > int64_t c_mtime; /* last modified time, seconds */ > int32_t c_extsize; /* external attribute size */ > int32_t c_spare4[6]; /* old block pointers */ > - u_int32_t c_file_flags; /* status flags (chflags) */ > + fflags_t c_file_flags; /* status flags (chflags) */ The struct represents on-disk protocol description, and thus shouldn't be changed. > int32_t c_spare5[2]; /* old blocks, generation number */ > u_int32_t c_uid; /* file owner */ > u_int32_t c_gid; /* file group */ > Index: lib/libc/sys/chflags.2 > =================================================================== > --- lib/libc/sys/chflags.2 (revision 217362) > +++ lib/libc/sys/chflags.2 (working copy) > @@ -42,11 +42,11 @@ > .In sys/stat.h > .In unistd.h > .Ft int > -.Fn chflags "const char *path" "u_long flags" > +.Fn chflags "const char *path" "fflags_t flags" > .Ft int > -.Fn lchflags "const char *path" "int flags" > +.Fn lchflags "const char *path" "fflags_t flags" > .Ft int > -.Fn fchflags "int fd" "u_long flags" > +.Fn fchflags "int fd" "fflags_t flags" > .Sh DESCRIPTION > The file whose name > is given by > Index: sbin/restore/tape.c > =================================================================== > --- sbin/restore/tape.c (revision 217362) > +++ sbin/restore/tape.c (working copy) > @@ -558,7 +558,7 @@ > int > extractfile(char *name) > { > - int flags; > + fflags_t flags; > uid_t uid; > gid_t gid; > mode_t mode; > Index: sys/kern/vfs_syscalls.c > =================================================================== > --- sys/kern/vfs_syscalls.c (revision 217362) > +++ sys/kern/vfs_syscalls.c (working copy) > @@ -96,7 +96,7 @@ > static int getutimes(const struct timeval *, enum uio_seg, struct timespec *); > static int setfown(struct thread *td, struct vnode *, uid_t, gid_t); > static int setfmode(struct thread *td, struct vnode *, int); > -static int setfflags(struct thread *td, struct vnode *, int); > +static int setfflags(struct thread *td, struct vnode *, fflags_t); > static int setutimes(struct thread *td, struct vnode *, > const struct timespec *, int, int); > static int vn_access(struct vnode *vp, int user_flags, struct ucred *cred, > @@ -2657,7 +2657,7 @@ > setfflags(td, vp, flags) > struct thread *td; > struct vnode *vp; > - int flags; > + fflags_t flags; > { > int error; > struct mount *mp; > @@ -2695,8 +2695,8 @@ > */ > #ifndef _SYS_SYSPROTO_H_ > struct chflags_args { > - char *path; > - int flags; > + char *path; > + fflags_t flags; > }; > #endif > int > @@ -2704,7 +2704,7 @@ > struct thread *td; > register struct chflags_args /* { > char *path; > - int flags; > + fflags_t flags; > } */ *uap; > { > int error; > @@ -2732,7 +2732,7 @@ > struct thread *td; > register struct lchflags_args /* { > char *path; > - int flags; > + fflags_t flags; > } */ *uap; > { > int error; > @@ -2757,8 +2757,8 @@ > */ > #ifndef _SYS_SYSPROTO_H_ > struct fchflags_args { > - int fd; > - int flags; > + int fd; > + fflags_t flags; > }; > #endif > int > @@ -2766,7 +2766,7 @@ > struct thread *td; > register struct fchflags_args /* { > int fd; > - int flags; > + fflags_t flags; > } */ *uap; > { > struct file *fp; > Index: sys/sys/stat.h > =================================================================== > --- sys/sys/stat.h (revision 217362) > +++ sys/sys/stat.h (working copy) > @@ -292,11 +292,11 @@ > #ifndef _KERNEL > __BEGIN_DECLS > #if __BSD_VISIBLE > -int chflags(const char *, unsigned long); > +int chflags(const char *, fflags_t); > #endif > int chmod(const char *, mode_t); > #if __BSD_VISIBLE > -int fchflags(int, unsigned long); > +int fchflags(int, fflags_t); > #endif > #if __POSIX_VISIBLE >= 200112 > int fchmod(int, mode_t); > @@ -306,7 +306,7 @@ > #endif > int fstat(int, struct stat *); > #if __BSD_VISIBLE > -int lchflags(const char *, int); > +int lchflags(const char *, fflags_t); > int lchmod(const char *, mode_t); > #endif > #if __POSIX_VISIBLE >= 200112 > Index: sys/ufs/ufs/dinode.h > =================================================================== > --- sys/ufs/ufs/dinode.h (revision 217362) > +++ sys/ufs/ufs/dinode.h (working copy) > @@ -140,7 +140,7 @@ > int32_t di_birthnsec; /* 76: Inode creation time. */ > int32_t di_gen; /* 80: Generation number. */ > u_int32_t di_kernflags; /* 84: Kernel flags. */ > - u_int32_t di_flags; /* 88: Status flags (chflags). */ > + fflags_t di_flags; /* 88: Status flags (chflags). */ Please revert for the same reason. > int32_t di_extsize; /* 92: External attributes block. */ > ufs2_daddr_t di_extb[NXADDR];/* 96: External attributes block. */ > ufs2_daddr_t di_db[NDADDR]; /* 112: Direct disk blocks. */ > Index: tools/regression/pjdfstest/pjdfstest.c > =================================================================== > --- tools/regression/pjdfstest/pjdfstest.c (revision 217362) > +++ tools/regression/pjdfstest/pjdfstest.c (working copy) > @@ -578,12 +577,14 @@ > break; > #ifdef HAS_CHFLAGS > case ACTION_CHFLAGS: > - rval = chflags(STR(0), (unsigned long)str2flags(chflags_flags, STR(1))); > + rval = chflags(STR(0), > + (fflags_t)str2flags(chflags_flags, STR(1))); > break; > #endif > #ifdef HAS_LCHFLAGS > case ACTION_LCHFLAGS: > - rval = lchflags(STR(0), (int)str2flags(chflags_flags, STR(1))); > + rval = lchflags(STR(0), > + (fflags_t)str2flags(chflags_flags, STR(1))); > break; > #endif > case ACTION_TRUNCATE: > _______________________________________________ > freebsd-fs@freebsd.org mailing list > http://lists.freebsd.org/mailman/listinfo/freebsd-fs > To unsubscribe, send any mail to "freebsd-fs-unsubscribe@freebsd.org"
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20110118041447.GA2087>