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

Groups > linux.debian.bugs.dist > #814972

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

Show all headers | View raw

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

Back to linux.debian.bugs.dist | Previous | Next — Previous in thread | Find similar | Unroll thread

Thread

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

csiph-web