bug-hurd
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH] Change file_utimes RPC to use a struct timespec and update t


From: Samuel Thibault
Subject: Re: [PATCH] Change file_utimes RPC to use a struct timespec and update the servers to use UTIME_NOW and UTIME_OMIT.
Date: Sun, 13 Sep 2015 13:49:08 +0200
User-agent: Mutt/1.5.21+34 (58baf7c9f32f) (2010-12-30)

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



reply via email to

[Prev in Thread] Current Thread [Next in Thread]