Date: Thu, 1 Mar 2012 00:08:28 +1100 (EST) From: Bruce Evans <brde@optusnet.com.au> To: Sergey Kandaurov <pluknet@gmail.com> Cc: Jilles Tjoelker <jilles@FreeBSD.org>, d@delphij.net, freebsd-arch@FreeBSD.org Subject: Re: RFC: futimens(2) and utimensat(2) Message-ID: <20120229232250.G3812@besplex.bde.org> In-Reply-To: <CAE-mSOJU=hm8%2B-AC_oQmx%2Bh2grv7PGaH7kNYKoT3GMePDPXsYg@mail.gmail.com> References: <4F4DC876.3010809@delphij.net> <CAE-mSOJU=hm8%2B-AC_oQmx%2Bh2grv7PGaH7kNYKoT3GMePDPXsYg@mail.gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
This message is in MIME format. The first part should be readable text, while the remaining parts are likely unreadable without MIME-aware tools. --0-1303333959-1330520908=:3812 Content-Type: TEXT/PLAIN; charset=X-UNKNOWN; format=flowed Content-Transfer-Encoding: QUOTED-PRINTABLE On Wed, 29 Feb 2012, Sergey Kandaurov wrote: > On 29 February 2012 10:40, Xin Li <delphij@delphij.net> wrote: >> >> These are required by IEEE Std 1003.1-2008. =A0Patchset at: >> >> http://people.freebsd.org/~delphij/for_review/utimens.diff I didn't look at this because it wasn't in the mail :-). > This is the older version last time discussed with jilles. > It misses man page update and compat32 parts (both were > done since then except missing ERROR section in utimes(2). > e.g. my compat32 version is just as yours :)). > I started to commit my version (you can see r227447) but > failed due to missing ERROR section, my lack of english to > rewrite utimes(2) man page, and too complicated and wrong > ERROR section in the existing utimes(2). > > http://plukky.net/~pluknet/patches/utimes.2008.3.diff > > It is pretty similar to your except I done getutimens() a bit different. > I had to introduce such complication to pass all tests. > Take note on private flags UTIMENS_NULL and UTIMENS_EXIT. > > Index: sys/kern/vfs_syscalls.c > =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D > --- sys/kern/vfs_syscalls.c=09(revision 220831) > +++ sys/kern/vfs_syscalls.c=09(working copy) > ... > static int > +getutimens(usrtsp, tspseg, tsp, retflags) > +=09const struct timespec *usrtsp; > +=09enum uio_seg tspseg; > +=09struct timespec *tsp; > +=09int *retflags; Should probably not use K&R function definitions in new code. > +{ > +=09int error; > +=09struct timespec tsnow; Structs should be sorted before scalars (and pointers). > + > +=09vfs_timestamp(&tsnow); Not used in all paths. > +=09*retflags =3D 0; Not used in all paths? > +=09if (usrtsp =3D=3D NULL) { > +=09=09tsp[0] =3D tsnow; > +=09=09tsp[1] =3D tsnow; > +=09=09*retflags |=3D UTIMENS_NULL; > +=09=09return (0); > +=09} > +=09if (tspseg =3D=3D UIO_SYSSPACE) { > +=09=09tsp[0] =3D usrtsp[0]; > +=09=09tsp[1] =3D usrtsp[1]; > +=09} else if ((error =3D copyin(usrtsp, tsp, sizeof(*tsp) * 2)) !=3D 0) > +=09=09=09return (error); Indentation. > + Extra blank line. Many more of these below. > +=09if (tsp[0].tv_nsec =3D=3D UTIME_OMIT && tsp[1].tv_nsec =3D=3D UTIME_O= MIT) > +=09=09*retflags |=3D UTIMENS_EXIT; > +=09if (tsp[0].tv_nsec =3D=3D UTIME_NOW && tsp[1].tv_nsec =3D=3D UTIME_NO= W) > +=09=09*retflags |=3D UTIMENS_NULL; > + > +=09if (tsp[0].tv_nsec =3D=3D UTIME_OMIT) > +=09=09tsp[0].tv_sec =3D VNOVAL; tsp[0].tv_nsec is not initialized (except it is UTIME_OMIT, which might be the same as VNOVAL). The patch seems to be missing the header part that defines UTIME_OMIT). Most setattr vnops are sloppy about checking both tv_sec and tv_nsec, but VATTR_NULL() sets both to VNOVAL for setattrs that don't request a time change. More care is actually required in the opposite direction -- getattr defaults va_birthtime. tv_sec.tv_nsec to -1.0, so that when a getattr doesn't understand birthime it comes back back unchanged as -1.0 which gives the error value (time_t)-1. All attributes for getattr should be defaulted like this so that all file systems don't have to know about them, but only va_birthtime, va_fsid and va_rdev are (all the others default to stack garbage). > +=09else if (tsp[0].tv_nsec =3D=3D UTIME_NOW) > +=09=09tsp[0] =3D tsnow; > +=09else if (tsp[0].tv_nsec < 0 || tsp[0].tv_nsec >=3D 1000000000L) > +=09=09return (EINVAL); > + > +=09if (tsp[1].tv_nsec =3D=3D UTIME_OMIT) > +=09=09tsp[1].tv_sec =3D VNOVAL; > +=09else if (tsp[1].tv_nsec =3D=3D UTIME_NOW) > +=09=09tsp[1] =3D tsnow; > +=09else if (tsp[1].tv_nsec < 0 || tsp[1].tv_nsec >=3D 1000000000L) > +=09=09return (EINVAL); Is it possible to extend this API to support birthtimes (and with more security control, ctimes)? Encoding more in tv_nsec should do it. Certain magic values in tsp[1].tv_nsec would indicate that there are more than 2 entries in tsp[]. An extra copyin is needed to read the extra entries (after reading tsp[1] to see if there are more). Better add this before the ABI solidifies. This would have worked for utimes() too, with with magic in tsp[1].tv_usec, but this seems unnecessary now. Bruce --0-1303333959-1330520908=:3812--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20120229232250.G3812>