Inconsistency and bug with *chflags functions
Gleb Kurtsou
gleb.kurtsou at gmail.com
Tue Jan 18 04:40:21 UTC 2011
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 at freebsd.org mailing list
> http://lists.freebsd.org/mailman/listinfo/freebsd-fs
> To unsubscribe, send any mail to "freebsd-fs-unsubscribe at freebsd.org"
More information about the freebsd-fs
mailing list