On Wed, 4 Jul 2007, Nathan Scott wrote:
> On Tue, 2007-07-03 at 20:19 +1000, Michael Newton wrote:
> > i decided to repost as ive tried to take on board favouring
> > readability &
> > conciseness over keeping the diffs short
>
> <paininbuttmode>
> This isn't quite right still, and I'm confused as to why you are
> looking for a halfway point between the old code and the simpler
> version that I posted? In particular:
> </paininbuttmode>
>
> >
> > ===========================================================================
> > mgmt/pcp/src/pmcd/rc_pcp
> > ===========================================================================
> >
> > --- a/mgmt/pcp/src/pmcd/rc_pcp 2007-07-03 20:16:14.000000000 +1000
> > +++ b/mgmt/pcp/src/pmcd/rc_pcp 2007-07-03 19:29:52.089858288 +1000
> > @@ -383,16 +383,19 @@
> > fi
> > $ECHO $PCP_ECHO_N "Waiting for PMCD to
> > terminate ...""$PCP_ECHO_C"
> > gone=0
> > - for i in 1 2 3 4 5 6
> > + delay=200
> > + while [ $i -lt $delay ]
> > do
> > - sleep 3
> > + # dont sleep before 1st pid check, or after last
> > + [ $i -eq 0 ] || pmsleep 0.1
> > + i=`expr $i + 1`
>
> This is what I meant with "halfway" - this keeps $i for no reason
> AFAICT (am i missing something?) - $i was the 123456 loop counter
> before, but now that we can control the loop using $delay, it's
> redundant.
>
> And why keep the sleep at the top of the loop, with that extra zero
> check, instead of having it as the end of the loop, and skipping out
> if we've started the process?
<longsufferingmode>
its so that you: "# dont sleep before 1st pid check, or after last"
..your version continues to have a final sleep which is not followed by a
check (in this case, of whether the proc has exited). Thats just a delay
to no effect. If its "halfway", its because i tried to avoid that
problem, but also get rid of "while :". Im not greatly fond of a
conditional thats only going to trigger the 1st time through, but given
that all its protecting is a delay, it doesnt seem too bad.
I find it strange that you are so keen (quite rightly, im sure!) on
ditching the sleep-before-return that youve just announced the patch to
remove, but keep putting it in in the body of the loop!
youre right of course it cant be $delay %10 in my version: thats bad.
Also its true i could get by with 2 variables instead of 3.. but to reduce
it to one, its back to "while :". You can have
(1) while : + 1 variable + sleep at the end OR u can have
(2) while [ delay cond ] with a variable protecting the sleep
at the start. Trying to get both is going to be even sillier IMHO.
I'll stop trying to 2nd guess what youre going to like (at which i am
manifestly failing) and go with my own
feeling that (1) is more readable than (2)
arguably using a special purpose $gone is more robust from a maintenance
PoV. For instance, if i changed my code only by removing $gone [and
changing $delay %10 to $i %10 of course!], it would technically be
incorrect in that if i happen to have found the pid gone on the last
iteration, it would say the process didnt die.. id need to also move the
$i inc down after the break.. a kind of maintenance error ive seen plenty
times. Still, its a bit swings & roundabouts.. i wouldnt stress to defend
that one
> > + $VERBOSE && [ `expr $delay % 10` -eq 0 ] && $PCP_ECHO_PROG
> > $PCP_ECHO_N ".""$PCP_ECHO_C"
>
> Little bug there - $delay is a constant, in your version, so
> you will only ever print one '.'. In my version, delay was
> used to count down to zero, so using $delay there was valid
> for my patch, but not yours.
>
> > _get_pids_by_name pmcd >$tmp.tmp
> > if [ ! -s $tmp.tmp ]
> > then
> > gone=1
> > break
> > fi
> > - $ECHO $PCP_ECHO_N ".""$PCP_ECHO_C"
> > done
> > if [ $gone != 1 ] # It just WON'T DIE, give up.
>
> And when using $delay as the loop control, we can also remove
> the special $gone boolean flag, since we always know at the
> end of the loop which way it ended.
>
> Was there something not working in my patch that it needed to
> be reworked? This is what these loops should look like IMO
> (and they should have the same control structure in all of the
> scripts) - it is very easy to follow this loop structure,
> compared to the other way(s) with the additional variables:
>
> delay=200 # tenths of a second
> while [ $delay -gt 0 ]
> do
> if pmlock -v lock >$tmp.out
> then
> echo $dir/lock >$tmp.lock
> break
> else
> [ -f $tmp.stamp ] || touch -t `pmdate -30M %Y%m%d%H%M`
> $tmp.stamp
> if [ -z "`find lock -newer $tmp.stamp -print
> 2>/dev/null`" ]
> then
> echo "$prog: Warning: removing lock file older than
> 30 minutes"
> LC_TIME=POSIX ls -l $dir/lock
> rm -f lock
> fi
> fi
> pmsleep 0.1
> delay=`expr $delay - 1`
> done
>
> if [ $delay -eq 0 ]
>
>
> Also, as done above, this code should be removed entirely:
>
> + if uname -r | grep '^5\.3' >/dev/null
> + then
> + # IRIX 5.3 does not support -t for touch(1)
> + #
> + touch `pmdate -30M %m%d%H%M%y` $tmp.stamp
> + else
ok
> It was OK when IRIX was the only platform, but now any platform
> that has a 5.3 version will get caught in that code accidentally.
> And IRIX 5.3 is ancient history, so just toss this special case.
> I'll update my git tree with that patch shortly.
..which patch also removes the sleep-before-returns.. so does that imply
youve been able to ask ken about it already?
or you'll just back it out (or find another solution) if he tells you it
was actually achieving something?
# longsufferingmode continues indefinitely..
Dr.Michael("Kimba")Newton kimbrr@xxxxxxx
|