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

[dana <dana@xxxxxxx>]

On 4 Jan 2020, at 12:47, Cedric Ware <cedric.ware__bml@xxxxxxxxxxxxxx> wrote:
> This is a re-send/update of a patch I'd proposed some time ago

Feedback off the top of my head (haven't tested and don't know whether anyone
else would object to the change):

> +seconds; fractional seconds are allowed.  The shell will attempt
> +to lock the file every var(period) seconds during this period:
> +once a second by default, or according to the  value set by option
> +tt(-p).

The first thing that occurs to me is that you've now decoupled the polling
interval and time-out, which means that not only can you *manually* specify a
polling interval greater than the time-out, it's very possible for the
*default* interval to be greater than the time-out. In fact any -t value less
than one second is kind of pointless unless you also give -p

I'm not sure how you'd address this functionality-wise. Should it be an error?
idk. It should at least be mentioned in the documentation, though

Also this addition is worded a bit strangely i think. period is an argument to
-p; it isn't used unless -p is. Maybe something like: 'During this period, the
shell will attempt to lock the file every period seconds if the -p option is
given, otherwise once a second.' Something to that effect anyway

> +    long timeout_retry = 1e6;
> +    mnumber timeout_param;

It's kind of confusing that these are both named 'timeout' when they don't
specifically have anything to do with the time-out

> +		zlong timeout_retry_tmp = timeout_param.u.d * 1e6;

zsh mandates C89, you can't declare variables in the middle of a block

> +		timeout_retry = (timeout_retry_tmp > LONG_MAX) ?
> +		    LONG_MAX : timeout_retry_tmp;

Should *this* kind of thing be an error? I guess there's not really any
error-checking in the existing option-handling code, though

> +time_clock_us(void)

Feels like a weird function name, but i don't have a better suggestion

btw, it would probably be ideal if shtimer (the variable the shell uses for
the time built-in, the SECONDS parameter, &c.) was monotonic as well. But
that's a separate thing

> Is there a more rigorous test framework for this kind of thing, or
> anything else to be done that might help this patch be included?

The test stuff is in the Test directory in the repo. There don't seem to be
any existing tests for zsystem, so you would have to make like a
V13zsystem.ztst (see the other V* files for inspiration). Not sure what the
tests would actually do, though. Maybe you could call it with low -p/-t values
and use SECONDS to ensure that it times out roughly when it's supposed to?

dana