From owner-freebsd-arch@FreeBSD.ORG Wed Feb 29 13:08:43 2012 Return-Path: Delivered-To: freebsd-arch@FreeBSD.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 26661106566C; Wed, 29 Feb 2012 13:08:43 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from mail09.syd.optusnet.com.au (mail09.syd.optusnet.com.au [211.29.132.190]) by mx1.freebsd.org (Postfix) with ESMTP id AFCF98FC18; Wed, 29 Feb 2012 13:08:41 +0000 (UTC) Received: from c211-30-171-136.carlnfd1.nsw.optusnet.com.au (c211-30-171-136.carlnfd1.nsw.optusnet.com.au [211.30.171.136]) by mail09.syd.optusnet.com.au (8.13.1/8.13.1) with ESMTP id q1TD8SGV017994 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Thu, 1 Mar 2012 00:08:29 +1100 Date: Thu, 1 Mar 2012 00:08:28 +1100 (EST) From: Bruce Evans X-X-Sender: bde@besplex.bde.org To: Sergey Kandaurov In-Reply-To: Message-ID: <20120229232250.G3812@besplex.bde.org> References: <4F4DC876.3010809@delphij.net> MIME-Version: 1.0 Content-Type: MULTIPART/MIXED; BOUNDARY="0-1303333959-1330520908=:3812" Cc: Jilles Tjoelker , d@delphij.net, freebsd-arch@FreeBSD.org Subject: Re: RFC: futimens(2) and utimensat(2) X-BeenThere: freebsd-arch@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: Discussion related to FreeBSD architecture List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 29 Feb 2012 13:08:43 -0000 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 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--