From owner-freebsd-fs@FreeBSD.ORG Tue Jul 22 14:56:57 2008 Return-Path: Delivered-To: freebsd-fs@FreeBSD.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id E54BF106564A for ; Tue, 22 Jul 2008 14:56:57 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from fallbackmx06.syd.optusnet.com.au (fallbackmx06.syd.optusnet.com.au [211.29.132.8]) by mx1.freebsd.org (Postfix) with ESMTP id 7EF8C8FC25 for ; Tue, 22 Jul 2008 14:56:57 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from mail06.syd.optusnet.com.au (mail06.syd.optusnet.com.au [211.29.132.187]) by fallbackmx06.syd.optusnet.com.au (8.13.1/8.13.1) with ESMTP id m6MCfH99015144 for ; Tue, 22 Jul 2008 22:41:17 +1000 Received: from c220-239-252-11.carlnfd3.nsw.optusnet.com.au (c220-239-252-11.carlnfd3.nsw.optusnet.com.au [220.239.252.11]) by mail06.syd.optusnet.com.au (8.13.1/8.13.1) with ESMTP id m6MCfCn3024753 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Tue, 22 Jul 2008 22:41:13 +1000 Date: Tue, 22 Jul 2008 22:41:12 +1000 (EST) From: Bruce Evans X-X-Sender: bde@delplex.bde.org To: Jaakko Heinonen In-Reply-To: <20080722075718.GA1881@a91-153-120-204.elisa-laajakaista.fi> Message-ID: <20080722215249.K17453@delplex.bde.org> References: <200806020800.m528038T072838@freefall.freebsd.org> <20080722075718.GA1881@a91-153-120-204.elisa-laajakaista.fi> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed Cc: freebsd-fs@FreeBSD.org, ighighi@gmail.com Subject: Re: birthtime initialization 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, 22 Jul 2008 14:56:58 -0000 On Tue, 22 Jul 2008, Jaakko Heinonen wrote: > On 2008-06-02, Bruce Evans wrote: > [about patch for ext2fs in PR kern/122047] >> % + vap->va_birthtime.tv_sec = 0; >> % + vap->va_birthtime.tv_nsec = 0; >> >> This is unrelated and should be handled centrally. Almost all file >> systems get this wrong. Most fail to set va_birthtime, so stat() >> returns kernel stack garbage for st_birthtime. ffs1 does the same >> as the above. msdosfs does the above correctly, by setting tv_sec to >> (time_t)-1 in unsupported cases. > > How about this patch? > > %%% > Index: sys/kern/vfs_vnops.c > =================================================================== > --- sys/kern/vfs_vnops.c (revision 180588) > +++ sys/kern/vfs_vnops.c (working copy) > @@ -703,6 +703,9 @@ vn_stat(vp, sb, active_cred, file_cred, > #endif > > vap = &vattr; > + /* Not all file systems initialize birthtime. */ > + VATTR_NULL(vap); > + > error = VOP_GETATTR(vp, vap, active_cred, td); > if (error) > return (error); I want to initialize va_birthtime to { -1, 0 } here only. Don't initialize the whole vattr here. VOP_GETTATR() is supposed to initalize everything, but doesn't for va_birthtime. If there any other fields that VOP_GETTATR() doesn't initialize, then these should be searched for and fixed instead of setting them to the garbage value given by vattr_null. Similarly, if there are any fields that aren't supported by most file systems, then they should be searched for and defaulted like va_birthtime instead of requiring indivual file systems to invent a default value for them. > Index: sys/ufs/ufs/ufs_vnops.c > ... > Index: sys/fs/msdosfs/msdosfs_vnops.c > ... > Index: sys/nfsclient/nfs_subs.c There are a probably more file systems that have missing or slightly incorrect (all zero) settings of va_birthtime. > The patch adds VATTR_NULL() call to vn_stat() to initialize the vattr > structure before VOP_GETATTR() call. VATTR_NULL() initializes > va_birthtime.tv_sec and va_birthtime.tv_nsec to -1 (VNOVAL). I also > changed UFS1 and msdosfs to use consistent values. NFS needs explicit > initialization because otherwise values would be set to 0 due to memory > obtained with M_ZERO flag. VNOVAL = -1 only accidentally gives the correct value for va_birthtime.tv_sec. It gives a wrong value for va_birthtime.tv_nsec. It is better to set va_birthtime.tv_sec explicitly to -1. This -1 is only accidentantally equal to VNOVAL. Fortunately, this accident doesn't prevent VOP_GETATTR() from setting va_birthtime, since VNOVAL is only magic for VOP_SETATTR(). phk replied (but didn't quote enough, so I merged this manually): >> Looks like something Kirk forgot to me. >> We want to macroize the NOVAL for timespec instead of spreading >> -1 casts all over. This isn't a problem for the "GET" interface since VNOVAL doesn't apply to it. Also, the casts of -1 aren't really needed. ufs_settattr() doesn't have them for time_t's, and vattr_null() doesn't have them for anything. The correctness of this depends on the type of time_t (and the other va field times). In userland we're supposed to cast -1 to time_t for error detection in mktime() etc. In userland, time_t can be any arithmetic time so it is possible for (time_t)-1 != -1. Even there, I think there is only a problem if time_t is an unsigned intergral type shorter than int. Compilers may warn about other cases. ufs_settatr() has the casts for va_bytes (bogus cast of va_bytes to int, which breaks its value), va_uid, va_gid and va_mode. For va_mode, there is a problem -- the same one as in my example for time_t above -- va_mode is u_short so it cannot equal -1 (after the default promotions) except on exotic systems. For va_uid and va_gid, the casts were needed 15 years ago when uid_t and gid_t were 16 bits. I can't see any problem with omitting the cast for va_bytes -- va_bytes is u_quad_t, which is certainly at least as large as int, so it can equal VNOVAL = -1 after the default promotions though it cannot represent any negative value (now C's conversion rules requires (uquad_t)-1 == -1, and it would be a compiler bug to warn about expressions that depend on these rules). In vattr_null(), the assignments go the other way and VNOVAL = -1 always gets converted to the intended value (which is not always -1). C's conversion rules are depended on even more here to do something reasonable with (foo_t)-1. I wouldn't like VNOVAL being replaced by VNOTIMESPECVAL, VNOUIDVAL, ... etc. Recently I noticed a commit that replaced (struct foo *)0 by NULL together with less contentions replacements of plain 0 by NULL. Old code that tries to be careful uses (struct foo *)0 (or a macro NULLFOO for this) too much. Now that NULL is Standard we can just use plain NULL. Similarly for plain VNOVAL except in a few cases where -1 doesn't get converted right. Bruce