Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.debian.bugs.dist > #814931 > unrolled thread

Bug#858555: chiark-utils-bin: [patch] add timeout option to with-lock-ex

Back to article view | Back to linux.debian.bugs.dist

Contents

  Bug#858555: chiark-utils-bin: [patch] add timeout option to with-lock-ex Matthew Vernon <matthew@debian.org> - 2017-03-23 13:30 +0100
    Bug#858555: chiark-utils-bin: [patch] add timeout option to with-lock-ex Ian Jackson <ijackson@chiark.greenend.org.uk> - 2017-03-23 16:20 +0100
      Bug#858555: chiark-utils-bin: [patch] add timeout option to with-lock-ex Ian Jackson <ijackson@chiark.greenend.org.uk> - 2017-03-23 16:20 +0100
        Bug#858555: chiark-utils-bin: [patch] add timeout option to with-lock-ex Matthew Vernon <matthew@debian.org> - 2017-03-23 18:30 +0100
          Bug#858555: chiark-utils-bin: [patch] add timeout option to with-lock-ex Ian Jackson <ijackson@chiark.greenend.org.uk> - 2017-03-23 19:40 +0100
            Bug#858555: chiark-utils-bin: [patch] add timeout option to with-lock-ex Matthew Vernon <matthewv@chiark.greenend.org.uk> - 2017-03-24 14:40 +0100
      Bug#858555: chiark-utils-bin: [patch] add timeout option to with-lock-ex Ian Jackson <ijackson@chiark.greenend.org.uk> - 2017-03-23 17:00 +0100
      Bug#858555: chiark-utils-bin: [patch] add timeout option to with-lock-ex Matthew Vernon <matthew@debian.org> - 2017-03-23 17:10 +0100

#814931 — Bug#858555: chiark-utils-bin: [patch] add timeout option to with-lock-ex

[Multipart message — attachments visible in raw view] — view raw

Package: chiark-utils-bin
Version: 4.2.0
Severity: wishlist
Tags: upstream patch

Hi,

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.

-- System Information:
Debian Release: 7.11
  APT prefers oldstable-updates
  APT policy: (500, 'oldstable-updates'), (500, 'oldstable')
Architecture: i386 (i686)

Kernel: Linux 3.2.0-4-686-pae (SMP w/4 CPU cores)
Locale: LANG=en_GB.UTF-8, LC_CTYPE=en_GB.UTF-8 (charmap=UTF-8)
Shell: /bin/sh linked to /bin/dash

Versions of packages chiark-utils-bin depends on:
ii  libc6  2.13-38+deb7u11

Versions of packages chiark-utils-bin recommends:
ii  libc6       2.13-38+deb7u11
ii  libgmp10    2:5.0.5+dfsg-2
ii  libnettle4  2.4-3+deb7u1

Versions of packages chiark-utils-bin suggests:
ii  libx11-6  2:1.5.0-1+deb7u4

-- no debconf information

[toc] | [next] | [standalone]

#814963

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.

[toc] | [prev] | [next] | [standalone]

#814964

Ian Jackson writes ("Re: Bug#858555: chiark-utils-bin: [patch] add timeout option to with-lock-ex"):
> ITYM
>    if (*endptr || endptr=argv[2] || errno==ERANGE)
                         ==

Ian.

[toc] | [prev] | [next] | [standalone]

#814988

[Multipart message — attachments visible in raw view] — view raw

Hi,

Revised patch attached. My commit message:

Use getopt for CLI parsing; simplify SIGALRM handling

Using getopt for option parsing means that the ordering of -t and -f / 
-q doesn't matter, and we can put more error-handling of user input into 
the same place.

We now assume:
i) SIGALRM hasn't come from some other source
ii) SIGALRM was set to SIG_DFL on entry, and will be reset thus by execvp

...and this means we can simplify the code somewhat (no need to track 
elapsed time or store the old sigaction).

I'll point you at a git branch out-of-band.

Regards,

Matthew

[toc] | [prev] | [next] | [standalone]

#814999

Matthew Vernon writes ("Bug#858555: chiark-utils-bin: [patch] add timeout option to with-lock-ex"):
> Revised patch attached. My commit message:

Thanks.

We (Mark and I) had a long discussion on irc, including after you
left, about signal handling.  Our conclusion is that you need to do:

  sigprocmask(SIG_BLOCK); sigaction; setitimer; ... other setup ...;

  for (;;) {

    stuff

    sigprocmask(SIG_UNBLOCK);
    r = fcntl;
    sigprocmask(SIG_BLOCK);
    do things with r and errno

    stuff
  }

  setitmer(cancel)
  sigprocmask(SIG_SETMASK);
  /* might still get a timeout signal handler entry here! */
  sigaction(SIG_DFL); 

  exec;

And, as you suggested, all the if (alarmed) stuff then moved into the
signal handler.

Also, to make life extra annoying, I'd like this program not to mess
with the signals at all unless it has to, so all of this should be
conditional on secs.

Ian.

[toc] | [prev] | [next] | [standalone]

#815132

[Multipart message — attachments visible in raw view] — view raw

Hi,

Revised patch attached [also the out-of-band branch is updated]. As
suggested, the exiting-when-timedout logic is now done in the signal
handler, and I make sure not to call any unsafe functions while that
signal handler is in effect.

The signal-faffing is all conditional on secs (so won't happen if -t
isn't specified).

I don't call fail() when the signal handler is in effect and ALRM might
be unblocked, as that would be UB; one might wish to abort() or exit()
if the relevant calls failed?

Regards,

Matthew

[toc] | [prev] | [next] | [standalone]

#814970

Matthew Vernon writes ("Re: Bug#858555: chiark-utils-bin: [patch] add timeout option to with-lock-ex"):
> >> +	} else alarmed=0;
> >> +      }
> >
> > What ?  (Also, ewww at your coding style.)
> 
> I was considering the possibility of a SIGALRM coming from somewhere 
> else before the timer had expired (at which point, you'd go round the 
> loop again, and want alarmed=0 until our itimer expired).

Oh.  That would have been racy.  To cope with spurious SIGALRMs would
involve a lot of pratting abouot with sigprocmask etc.  Let's not.

Thanks,
Ian.

[toc] | [prev] | [next] | [standalone]

#814972

Hi,

> 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.

Yes, I was trying to avoid introducing full command-line parsing hassle, 
but I think if you want -t before as well as after -q/-f it'll be necessary.

> 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.

I'll have a think about it - I'm not sure this approach is simpler than 
the one I wrote.

> 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.

Allowing this assumption (and that SIGALRM was SIG_DFL on entry), 
however, will both simplify the code somewhat.

>> +      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.

Yeah, I think if moving to getopt or similar anyway, the error-handling 
can be done more in one place.

>> +    /*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.

Meh, I just write chatty code. It can go.

[snip style points; I'll do them]

>> +	} else alarmed=0;
>> +      }
>
> What ?  (Also, ewww at your coding style.)

I was considering the possibility of a SIGALRM coming from somewhere 
else before the timer had expired (at which point, you'd go round the 
loop again, and want alarmed=0 until our itimer expired).

Regards,

Matthew

[toc] | [prev] | [standalone]

Back to top | Article view | linux.debian.bugs.dist

csiph-web