Date: Tue, 9 Sep 2008 21:10:17 +1000 (EST) From: Bruce Evans <brde@optusnet.com.au> To: Jaakko Heinonen <jh@saunalahti.fi> Cc: freebsd-fs@freebsd.org Subject: Re: birthtime initialization Message-ID: <20080909204354.Q3089@besplex.bde.org> In-Reply-To: <20080903160736.GA12587@a91-153-122-179.elisa-laajakaista.fi> References: <200806020800.m528038T072838@freefall.freebsd.org> <20080722075718.GA1881@a91-153-120-204.elisa-laajakaista.fi> <20080722215249.K17453@delplex.bde.org> <20080723103424.GA1856@a91-153-120-204.elisa-laajakaista.fi> <20080724000618.Q16961@besplex.bde.org> <20080725072314.GA807@a91-153-120-204.elisa-laajakaista.fi> <20080725192315.D27178@delplex.bde.org> <20080903160736.GA12587@a91-153-122-179.elisa-laajakaista.fi>
next in thread | previous in thread | raw e-mail | index | archive | help
On Wed, 3 Sep 2008, Jaakko Heinonen wrote: > I further updated the patch. > > On 2008-07-25, Bruce Evans wrote: >> On Fri, 25 Jul 2008, Jaakko Heinonen wrote: >>> On 2008-07-24, Bruce Evans wrote: >>>> First, the fields shouldn't be initialized using VATTR_NULL() in >>>> VOP_GETATTR(). > > I removed VATTR_NULL() from xfs, mqueuefs, pseudofs, tmpfs, devfs > fdescfs and vattr_null() from devfs and portalfs. I also removed zeroing > from nfs. > >>> What do you think that is a proper default value for va_rdev? Some file >>> systems set it to 0 and some to VNOVAL. >> >> Either NODEV or VNOVAL explicitly translated late to NODEV. > > I changed following file systems to initialize va_rdev to NODEV instead > of 0 or VNOVAL (when appropriate): mqueuefs, tmpfs, portalfs, smbfs, > ntfs, fdescfs and msdosfs. Also in vn_stat() va_rdev is now initialized > to VNOVAL and explicitly translated to NODEV after the VOP_GETATTR() > call. > > I have tested the patch with these file systems: UFS2, UFS1, ext2fs, > ntfs, cd9660, udf, procfs, devfs, xfs, reiserfs, fdescfs, msdosfs, > mqueuefs, nfs, smbfs, portalfs. I like this version, but didn't check many details. > Index: sys/kern/vfs_vnops.c > =================================================================== > --- sys/kern/vfs_vnops.c (revision 182592) > +++ sys/kern/vfs_vnops.c (working copy) > @@ -703,6 +703,17 @@ vn_stat(vp, sb, active_cred, file_cred, > #endif > > vap = &vattr; > + > + /* > + * Initialize defaults for new and unusual fields, so that file > + * systems which don't support these fields don't need to know > + * about them. > + */ > + vap->va_birthtime.tv_sec = -1; > + vap->va_birthtime.tv_nsec = 0; > + vap->va_fsid = VNOVAL; > + vap->va_rdev = VNOVAL; Shouldn't this be NODEV? NODEV is ((dev_t)-1) and VNOVAL is plain (-1), so their values are identical after assignment to va_rdev... > + > error = VOP_GETATTR(vp, vap, active_cred); > if (error) > return (error); > @@ -750,7 +761,10 @@ vn_stat(vp, sb, active_cred, file_cred, > sb->st_nlink = vap->va_nlink; > sb->st_uid = vap->va_uid; > sb->st_gid = vap->va_gid; > - sb->st_rdev = vap->va_rdev; > + if (vap->va_rdev == VNOVAL) > + sb->st_rdev = NODEV; > + else > + sb->st_rdev = vap->va_rdev; ... this change seems to have no effect on machines with 32-bit 2's complement ints and to be wrong on other machines. Assignment of VNOVAL to va_rdev changes its value from -1 to 0xFFFFFFFFU, so it can only compare equal to VNOVAL if int has the same size as dev_t or there is stronger magic. When the comparision is fixed to compare with the assigned value (dev_t)VNOVAL == (dev_t)(-1) == NODEV, it is clear that the change has no effect. Bruce
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20080909204354.Q3089>