Matthew Vernon writes ("Bug#858555: chiark-utils-bin: [patch] add timeout
option to with-lock-ex"):
> I wanted a timeout option to with-lock-ex. The attached patch provides
> this - you can see -t <secs> after -q or -f and with-lock-ex will then
> wait that long while trying to get the lock before quitting.
>
> I have built and tested the attached patch against the current master
> branch in git.
Thanks.
I was initially surprised that you had -t combine with -q or -f, but
not with -w. But I now see why and I don't see a better way.
However, I have some comments about details;
The argument parser should allow -t before -q/-f as well
as after. It might be easiest to replace it with myopt
or getopt or something.
It should accept -t 0 (meaning the same as no -t).
I think it might be better to do the exiting directly in the signal
handler. That would make the overall flow control logic less
fiddly perhaps.
This would mean setting up the SIGALRM handler immediately before
fcntl, and SIGALRM back to SIG_DFL immediately afterwards. You need
to be careful with your error handling in this case: do the actual
error check on fcntl's return value _after_ disabling SIGALRM, so you
don't risk reentering stdio or malloc or something. You can disable
SIGALRM with sigprocmask or by setting it to SIG_IGN.
FAOD this is a "please consider and decide", not a definite request
for change.
If you prefer to keep things as they are, you don't need to check the
time after discovering alarmed. It's fine to assume SIGALRM doesn't
happen for some other reason. Also then you don't need to reset
SIGALRM because execve will do it.
You should assume that SIGALRM is SIG_DFL on entry. You don't
therefore need to save it.
> + if (0 == strncmp(argv[1],"-t",2)) {
This `0 ==' construct is not my coding style. I would use `!'.
> + errno = 0;
> + secs = strtol(argv[2], &endptr, 0);
> + if ((errno == ERANGE && (secs == LONG_MAX || secs == LONG_MIN))
> + || (errno !=0 && secs == 0)) {
ITYM
if (*endptr || endptr=argv[2] || errno==ERANGE)
> + fail("parsing timeout value");
> + }
Maybe instead, it would be better to have a pre-patch which breaks the
existing bad usage message out into a function badusage(). Then there
needs to be less open-coded error handling in your new parsing.
But see also my comments about getopt.
> + /*Install SIGALRM handler, and set a timer for secs seconds from now*/
This comment is IMO redundant. If you want to keep it, please put
spaces inside the /* */ and also then you probably want to linewrap it
somehow.
> - mode=='w' ? F_SETLKW :
> + mode=='w' || secs > 0 ? F_SETLKW :
|| secs
would do, surely ?
> + if (alarmed==1) {
I prefer this style:
if (alarmed)
> + tend = time(NULL); if (tend==(time_t)-1) fail("getting current time");
> + if (tend-tstart >= secs) {
See comments above.
> + } else alarmed=0;
> + }
What ? (Also, ewww at your coding style.)
Ian.
