Date: Mon, 17 Jan 2011 16:32:18 -0800 From: Garrett Cooper <yanegomi@gmail.com> To: fs@freebsd.org Subject: Inconsistency and bug with *chflags functions Message-ID: <AANLkTintj_eM=aRF4pO8PcT8VXvVJ=Booxag%2BwBg3gaX@mail.gmail.com>
index | next in thread | raw e-mail
[-- Attachment #1 --]
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).
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.
[-- Attachment #2 --]
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) */
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). */
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:
help
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?AANLkTintj_eM=aRF4pO8PcT8VXvVJ=Booxag%2BwBg3gaX>