From owner-freebsd-fs@FreeBSD.ORG Tue Jan 18 04:40:21 2011 Return-Path: Delivered-To: fs@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 85D33106566B for ; Tue, 18 Jan 2011 04:40:21 +0000 (UTC) (envelope-from gleb.kurtsou@gmail.com) Received: from mail-fx0-f54.google.com (mail-fx0-f54.google.com [209.85.161.54]) by mx1.freebsd.org (Postfix) with ESMTP id 122718FC16 for ; Tue, 18 Jan 2011 04:40:20 +0000 (UTC) Received: by fxm16 with SMTP id 16so6881788fxm.13 for ; Mon, 17 Jan 2011 20:40:20 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=gamma; h=domainkey-signature:date:from:to:cc:subject:message-id:references :mime-version:content-type:content-disposition:in-reply-to :user-agent; bh=QkqRDc0oqru7h7O1OiKwSAAzGi8oJq2D0BOdFcxSGi4=; b=m3DicMiP8dNfNhuN29UMV8ONy+wn0w02+/81l1XUu9FF2Xuw3IUq747rMaGZpfdy2O g1h8pp5kJurlvfKAqQI/lIDbwFx0YRzECAof7yassFitn17cc1bXTU9Rv9LPEFz2GCHY bEn4vSsy5u4LEArBJplF2BQGYk0h9xauEvfc4= DomainKey-Signature: a=rsa-sha1; c=nofws; d=gmail.com; s=gamma; h=date:from:to:cc:subject:message-id:references:mime-version :content-type:content-disposition:in-reply-to:user-agent; b=ghUq/8HhNbkp4EZBEdWyXPQt4fwi72Z56ij057iBCaNru7FE9dNX+jd732hqFQDQND mB3utJdn3wX8tNGKiTBaRTA8RCmAmu6CA9un7p1V7HW8pOGBzRCLWIktgh1sM7XL5Coe t0ZwY8aHu4wUJrVq7tzaue2xUiW8EuN8k96Yo= Received: by 10.223.114.14 with SMTP id c14mr5564793faq.103.1295324109720; Mon, 17 Jan 2011 20:15:09 -0800 (PST) Received: from localhost (lan-78-157-92-5.vln.skynet.lt [78.157.92.5]) by mx.google.com with ESMTPS id 21sm1925616fav.41.2011.01.17.20.15.08 (version=SSLv3 cipher=RC4-MD5); Mon, 17 Jan 2011 20:15:09 -0800 (PST) Date: Tue, 18 Jan 2011 06:14:47 +0200 From: Gleb Kurtsou To: Garrett Cooper Message-ID: <20110118041447.GA2087@tops.skynet.lt> References: MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.21 (2010-09-15) Cc: fs@freebsd.org Subject: Re: Inconsistency and bug with *chflags functions X-BeenThere: freebsd-fs@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: Filesystems List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 18 Jan 2011 04:40:21 -0000 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"