Re: [PATCH] Enable sub-second timeout in zsystem flock

[Daniel Shahaf <d.s@xxxxxxxxxxxxxxxxxx>]

Cedric Ware wrote on Sat, 14 Mar 2020 22:04 +0100:
> +++ zsh-5.8/Doc/Zsh/mod_system.yo	2020-03-08 18:39:32.672683779 +0100
> @@ -166,7 +166,7 @@
>  printed in the last case, but the parameter tt(ERRNO) will reflect
>  the error that occurred.
>  )
> -xitem(tt(zsystem flock) [ tt(-t) var(timeout) ] [ tt(-f) var(var) ] [tt(-er)] var(file))
> +xitem(tt(zsystem flock) [ tt(-t) var(timeout) ] [ tt(-i) var(interval) ] [ tt(-f) var(var) ] [tt(-er)] var(file))
>  item(tt(zsystem flock -u) var(fd_expr))(

Please s/var(var)/var(interval)/ in the body paragraphs of the item()().

> +++ zsh-5.8/Src/Modules/system.c	2020-03-08 18:43:02.756951315 +0100

Please write patches against git HEAD rather than against the latest
release, if possible.  (See 45283 for an example where that would have
mattered.)

> @@ -583,7 +586,44 @@

(Aside: if your diff(1) supports the -p option to show function names
in hunk headers, please use it; it makes reviewing easier.)

>  		} else {  
>  		    optarg = *args++;
>  		}  
> -		timeout = mathevali(optarg);
> +		timeout_param = matheval(optarg);
> +		if (!(timeout_param.type & MN_FLOAT)) {
> +		    timeout_param.type = MN_FLOAT;
> +		    timeout_param.u.d = (double)timeout_param.u.l;
> +		}
> +		timeout_tmp = timeout_param.u.d * 1e6;
> +		if ((timeout_tmp < 1) || (timeout_tmp > ZLONG_MAX / 2)) {
> +		    zwarnnam(nam, "flock: invalid timeout value");

I think the invalid value should be included in the error message,
either as a number, or as a string if needed: "flock: invalid timeout
value: '%s'".  In my experience, "invalid input" error messages are
easier to consume when they actually say what the invalid input is.

dana, I see you advocated the opposite approach in 45283.  What's your
rationale?

If we change this, there's another instance of this in the next «case»
block that should be changed too.

> +		    return 1;
> +		}
> +		timeout = (zlong)timeout_tmp;
> +		break;

> @@ -647,7 +687,8 @@
>      lck.l_len = 0;  /* lock the whole file */
>  
>      if (timeout > 0) {
> -	time_t end = time(NULL) + (time_t)timeout;
> +	zlong now;
> +	zlong end = time_clock_us() + timeout;

Could this sum overflow (for example, if the -t option is used)?

>  	while (fcntl(flock_fd, F_SETLK, &lck) < 0) {
>  	    if (errflag) {
>                  zclose(flock_fd);

> @@ -658,11 +699,15 @@
>  		zwarnnam(nam, "failed to lock file %s: %e", args[0], errno);
>  		return 1;
>  	    }  
> -	    if (time(NULL) >= end) {
> +	    now = time_clock_us();
> +	    if (now >= end) {
>                  zclose(flock_fd);
>  		return 2;
>              }  
> -	    sleep(1);
> +	    if (now + timeout_retry > end) {

Could this sum overflow (for example, if the -i option is used)?

> +		timeout_retry = end - now;
> +	    }
> +	    zsleep(timeout_retry);

> +++ zsh-5.8/Src/utils.c	2020-03-08 18:43:02.756951315 +0100
> @@ -2745,6 +2745,26 @@
>  /*
> + * Return the current time in microseconds, using the system's
> + * monotonic clock if supported, the wall clock if not.
> + */
> +
> +/**/
> +zlong
> +time_clock_us(void)
> +{
> +#if defined(HAS_CLOCK_GETTIME) && defined(CLOCK_MONOTONIC)

Isn't the macro spelled HAVE_CLOCK_GETTIME?  It's spelled that way in
Src/compat.c:105.

Have both codepaths (the #if and the #else) been tested?

> +    struct timespec ts;
> +    clock_gettime(CLOCK_MONOTONIC, &ts);
> +    return ts.tv_sec * (zlong)1e6 + ts.tv_nsec / 1000;
> +#else
> +    struct timeval tv;
> +    gettimeofday(&tv, NULL);
> +    return tv.tv_sec * (zlong)1e6 + tv.tv_usec;
> +#endif
> +}  
> +

Cheers,

Daniel