From: Daniel Shahaf <d.s@daniel.shahaf.name>
To: Cedric Ware <cedric.ware__bml@normalesup.org>
Cc: dana <dana@dana.is>, <zsh-workers@zsh.org>
Subject: Re: [PATCH] Enable sub-second timeout in zsystem flock
Date: Sun, 15 Mar 2020 00:50:35 +0000 [thread overview]
Message-ID: <20200315005036.45bc846b@tarpaulin.shahaf.local2> (raw)
In-Reply-To: <20200314210454.hp562smyqv3ew255@phare.normalesup.org>
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
next prev parent reply other threads:[~2020-03-15 0:51 UTC|newest]
Thread overview: 33+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-07-29 20:35 Cedric Ware
2019-07-29 22:25 ` Bart Schaefer
2020-01-04 18:47 ` Cedric Ware
2020-01-05 18:42 ` dana
2020-01-05 21:49 ` dana
2020-01-06 17:30 ` Cedric Ware
2020-01-06 17:36 ` Peter Stephenson
2020-01-07 3:48 ` dana
2020-01-11 15:41 ` Cedric Ware
2020-01-11 19:36 ` dana
2020-01-12 4:25 ` dana
2020-03-08 18:39 ` Cedric Ware
2020-03-12 18:46 ` dana
2020-03-12 19:13 ` dana
2020-03-14 21:04 ` Cedric Ware
2020-03-15 0:50 ` Daniel Shahaf [this message]
2020-03-15 1:04 ` dana
2020-03-15 16:03 ` Cedric Ware
2020-03-15 16:54 ` Daniel Shahaf
2020-03-15 17:35 ` Peter Stephenson
2020-03-15 18:36 ` Cedric Ware
2020-03-15 19:13 ` Daniel Shahaf
2020-04-13 21:34 ` Cedric Ware
2020-04-14 11:47 ` Daniel Shahaf
2020-04-14 20:21 ` Cedric Ware
2020-04-15 1:15 ` Daniel Shahaf
2020-04-15 2:05 ` dana
2020-04-16 4:24 ` Daniel Shahaf
2020-04-18 16:32 ` Cedric Ware
2020-04-20 17:28 ` dana
2020-04-20 22:17 ` Cedric Ware
2020-03-15 1:04 ` Daniel Shahaf
2020-03-13 14:26 ` dana
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20200315005036.45bc846b@tarpaulin.shahaf.local2 \
--to=d.s@daniel.shahaf.name \
--cc=cedric.ware__bml@normalesup.org \
--cc=dana@dana.is \
--cc=zsh-workers@zsh.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://git.vuxu.org/mirror/zsh/
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).