Nathan Scott <nscott@xxxxxxxxxx>
>The SEE ALSO section could probably reference sleep(1) and nanosleep(2).
yep
>> ===========================================================================
>> mgmt/pcp/src/pmcd/rc_pcp
>> ===========================================================================
>>
>> --- a/mgmt/pcp/src/pmcd/rc_pcp 2007-06-29 18:09:45.000000000 +1000
>> +++ b/mgmt/pcp/src/pmcd/rc_pcp 2007-06-29 16:07:49.625951131 +1000
>> @@ -100,6 +100,8 @@
>> ;;
>> esac
>>
>> +SLEEPCMND="$PCP_BINADM_DIR/pmsleep 0.1"
>> +
>
>This variable just seems to be obfuscating the logic, I'd remove it.
>PCP_BINADM_DIR is set in the PATH in /etc/pcp.env, so full path isn't
>needed there. Since the 0.1 argument is important in several of the
>uses (as there is control flow assuming tenths of a second in places),
>the argument should be expanded close to the logic thats using it too.
if you like
>> _pmcd_logfile()
>> {
>> default=$RUNDIR/pmcd.log
>> @@ -383,16 +385,25 @@
>> fi
>> $ECHO $PCP_ECHO_N "Waiting for PMCD to
>> terminate ...""$PCP_ECHO_C"
>> gone=0
>> - for i in 1 2 3 4 5 6
>> + i=0
>> + j=0
>> + while :
>> do
>> - sleep 3
>> _get_pids_by_name pmcd >$tmp.tmp
>> if [ ! -s $tmp.tmp ]
>> then
>> gone=1
>> break
>> fi
>> - $ECHO $PCP_ECHO_N ".""$PCP_ECHO_C"
>> + i=`expr $i + 1`
>> + if [ $i -ge 10 ]
>> + then
>> + i=0
>> + [ $j -ge $delay ] && break
>> + j=`expr $j + 1`
>> + $ECHO $PCP_ECHO_N ".""$PCP_ECHO_C"
>> + fi
>> + $SLEEPCMND
>> done
>> if [ $gone != 1 ] # It just WON'T DIE, give up.
>
>Hmmm, thats not right. Firstly, $delay doesn't exist in this script
>(it looks like this script snippet has been incorrectly duplicated),
yes
>so you'll get a shell-syntax-error if that branch (with $j) is taken.
ok
>Secondly, that branch is b0rken in that it will only ever print one
>'.' character - its meant to print one dot every iteration (or every
>second now, I guess). You'll want to use an expr modulo (%) there.
dont understand. $i is incremented every time round. When it gets to 10
* it gets reset to zero
* j is incremented
* '.' is printed
so $i counts tenths of a sec and $j counts whole seconds
the structure is more complicated but it avoids the division..
swings & roundabouts?
>Thirdly, the while logic is strange - you could just use the normal
>loop control mechanism (rather than the "while :" and explicit break
>statements).
>
>(I've attached an alternate patch that implements these things).
as i said before, test at the top means that you are not testing the
condition which is the actual purpose of the loop (in this case whether
pmcd has gone away yet) after the final sleep.. so there wasnt much point
in doing it!
btw i also aimed to keep the diffs minimal
>> ===========================================================================
>> mgmt/pcp/src/pmie/pmie_check.sh
>> ===========================================================================
>
>> ===========================================================================
>> mgmt/pcp/src/pmlogctl/pmlogger_check.sh
>> ===========================================================================
>
>These two scripts have the same sorts of problems, fixed in attached
>patch.
>
>>
>> ===========================================================================
>> mgmt/pcp/src/pmsleep/pmsleep.c
>> ===========================================================================
>> ...
>> +int
>> +main(int argc, char **argv)
>> +{
>> + struct timespec rqt;
>> + struct timeval delta;
>> + int r = 0;
>> + char *msg;
>> +
>> + if (argc == 2) {
>> + if (pmParseInterval(argv[1], &delta, &msg) < 0) {
>> + fputs(msg, stderr);
>> + free(msg);
>> + } else {
>> + rqt.tv_sec = delta.tv_sec;
>> + rqt.tv_nsec = delta.tv_usec * 1000;
>> + if (0 != nanosleep(&rqt, NULL))
>> + r = errno;
>> +
>> + exit(r);
>> + }
>> + }
>> + fprintf(stderr, "Usage: pmsleep [-v] interval\n");
>
>There's no -v option.
oops!
> The 'r' variable isn't really necessary -
>see my attached patch that makes this slightly simpler.
very slightly! is there a reason to prefer "return" over "exit"?
> Was it intentional to print a usage message when a pmParseInterval error
>occurs? Seems a bit odd - *shrug*, not a big deal obviously.
yes it was intentional -- doesnt seem odd to me - why wouldnt you print one?
Its consistent with the behaviour of say pmval when given a malformed -t value
>BTW, one other thing I noticed here in the pmie and pmlogger
>check scripts - theres a comment near the start of the loop
>in _check_logger() that doesn't match what the code does.
>And the code looks wrong - the comment is:
>
># $logfile was previously removed, if it has appeared again
># then we know pmlogger has started ... if not just sleep and
># try again
>
>But what it actually seems to do is sleep when pmlogger has
>started, just prior to returning from the call (when we know
>pmlogger has started...).
of course i didnt originate this code but i think you have misinterpreted
it. Possibly the comment ought to be one step higher up the file, eg before
if [ -f $logfile ] in pmie_check.sh. There are 2 sleeps within the loop, one
of which is done every time, and one which is only done at the end. The
comment i believe refers to the one which is done every time, which is
lexically the 2nd of the 2. You are looking at the lexically 1st, which is
only done at the end. For instance in pmlogger_check, if you have succeeded in
making a connection, you know pmlogger is through its initialisation, but now
you need to give it a little time to finish cleaning up the connection you
have just relinquished, so as to be ready to talk to a real client. Thats my
theory anyway! In pmie_check perhaps the theory is there may still be little
init to be done after the log file has appeared -- or maybe the idiom was just
copied from pmlogger_check -- but anyway now that its only 0.1s i dont
think its doing any harm.
ps i really need to get this in
Dr.Michael("Kimba")Newton kimbrr@xxxxxxx
|