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?
> --- 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.
> +/* 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.
> 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.
> --- 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?
> 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
