Hi Samuel On Sun, 13 Sep 2015 at 12:49 Samuel Thibault <samuel.thiba...@gnu.org> wrote:
> Flávio Cruz, le Mon 31 Aug 2015 15:16:08 +0000, a écrit :
> > Note that I also added a two macros to convert between timespecs and
> > time_value_t in libshouldbeinlibc/timefmt.h (one was taken from the exec
> > server). Not sure if this is the best place.
>
> It seems a bit odd to put it in "timefmt" which means time
> format, indeed. Perhaps we should rather add them to gnumach's
> mach/time_value.h?
>
Agree. Moved to mach/time_value.h.
>
> > --- a/sysdeps/mach/hurd/futimens.c
> > +++ b/sysdeps/mach/hurd/futimens.c
> > {
> > - atime.seconds = tsp[0].tv_sec;
> > - atime.microseconds = tsp[0].tv_nsec / 1000;
> > - mtime.seconds = tsp[1].tv_sec;
> > - mtime.microseconds = tsp[1].tv_nsec / 1000;
> > + atime.tv_sec = tsp[0].tv_sec;
> > + atime.tv_nsec = tsp[0].tv_nsec;
> > + mtime.tv_sec = tsp[1].tv_sec;
> > + mtime.tv_nsec = tsp[1].tv_nsec;
>
> Mmm, better just atime=tsp[0]; mtime=tsp[1];?
>
> > --- a/sysdeps/mach/hurd/futimes.c
> > +++ b/sysdeps/mach/hurd/futimes.c
> > @@ -27,24 +27,44 @@
>
> > + if(tvp == NULL)
>
> Take care of formatting.
>
> > --- a/sysdeps/mach/hurd/lutimes.c
> > +++ b/sysdeps/mach/hurd/lutimes.c
>
> > + if(tvp == NULL)
>
> ditto, and probably other places as well.
>
Done.
>
> > +/* Implement file_utimens as described in <hurd/fs.defs>. */
> > +kern_return_t
> > +diskfs_S_file_utimens (struct protid *cred,
> > + struct timespec atime,
> > + struct timespec mtime)
> > +{
> > CHANGE_NODE_FIELD (cred,
> > - ({
> > - if (!(err = fshelp_isowner (&np->dn_stat,
> cred->user)))
> > + ({
> > + if (!(err = fshelp_isowner (&np->dn_stat, cred->user)))
>
> Please avoid reformatting indentation even if the existing is odd, it
> both makes reading patches clumsier, and makes git annotate to find
> changes harder to use.
>
> > + if (atime.tv_nsec == UTIME_NOW)
> > + np->dn_set_atime = 1;
> > + else if (atime.tv_nsec == UTIME_OMIT)
> > + np->dn_set_atime = 0;
>
> I don't think you want to set dn_set_atime to 0 in these places. If there
> was a
> previous utime call which used UTIME_NOW for instance, we don't want to
> interfere with that. I think in the UTIME_OMIT case we should just do
> nothing.
>
Gotcha.
>
> > diff --git a/libnetfs/file-utimes.c b/libnetfs/file-utimes.c
> > index 1915609..2bf22f7 100644
> > --- a/libnetfs/file-utimes.c
> > +++ b/libnetfs/file-utimes.c
>
> Here, rather put braces like this:
>
> > + if (atimein.tv_nsec == UTIME_NOW || mtimein.tv_nsec == UTIME_NOW)
>
> {
>
> > + maptime_read (netfs_mtime, &t);
> > +
> > + if (atimein.tv_nsec == UTIME_NOW)
> > + TIMEVAL_TO_TIMESPEC (&t, &atimein);
> > + if (mtimein.tv_nsec == UTIME_NOW)
> > + TIMEVAL_TO_TIMESPEC (&t, &mtimein);
>
> }
>
> which will be a bit more efficient, and avoid dumb compilers to emit
> warnings about t being undefined.
>
Done.
>
> > --- a/libtreefs/treefs-s-hooks.h
> > +++ b/libtreefs/treefs-s-hooks.h
> > @@ -53,7 +53,7 @@ DHH(s_file_chmod, error_t, mode_t)
> > DHH(s_file_chflags, error_t, int)
> > #define treefs_s_file_chflags(h, args...)
> \
> > _TREEFS_CHH(h, S_FILE_CHFLAGS, s_file_chflags , ##args)
> > -DHH(s_file_utimes, error_t, time_value_t, time_value_t)
> > +DHH(s_file_utimens, error_t, struct timespec, struct timespec)
> > #define treefs_s_file_utimes(h, args...)
> \
> > _TREEFS_CHH(h, S_FILE_UTIMES, s_file_utimes , ##args)
>
> This looks odd: don't we need both utimes and utimens?
>
I think we only need file_utimens. Translators that use libtreefs only need
to implement file_utimens. Furthermore, I didn't find any libtreefs based
translator, so we should have no backwards compatibility issue.
>
> > diff --git a/libtrivfs/times.c b/libtrivfs/times.c
> > index 5f08cb1..4c5a0e4 100644
> > --- a/libtrivfs/times.c
> > +++ b/libtrivfs/times.c
> > error_t
> > trivfs_set_atime (struct trivfs_control *cntl)
> > {
> > - io_stat (cntl->underlying, &st);
> > + mtime.tv_sec = 0;
> > + mtime.tv_nsec = UTIME_OMIT;
>
> that's a nice side effect :)
>
> > --- a/nfs/nfs.c
> > +++ b/nfs/nfs.c
> > + else
> > + *(p++) = DONT_CHANGE; /* no atime */
>
> also nice :)
>
> Samuel
>
I've attached the three updated patches.
Flavio
glibc-futimens.patch
Description: Binary data
gnumach-timespec-timevalue.patch
Description: Binary data
hurd-futimens.patch
Description: Binary data
