Date: Tue, 22 Jul 2008 22:41:12 +1000 (EST) From: Bruce Evans <brde@optusnet.com.au> To: Jaakko Heinonen <jh@saunalahti.fi> Cc: freebsd-fs@FreeBSD.org, ighighi@gmail.com Subject: Re: birthtime initialization Message-ID: <20080722215249.K17453@delplex.bde.org> In-Reply-To: <20080722075718.GA1881@a91-153-120-204.elisa-laajakaista.fi> References: <200806020800.m528038T072838@freefall.freebsd.org> <20080722075718.GA1881@a91-153-120-204.elisa-laajakaista.fi>
next in thread | previous in thread | raw e-mail | index | archive | help
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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20080722215249.K17453>