>>> +int main(int argc, char **argv)
>>> +{
>>> + unsigned long i;
>>> + struct passwd *ent;
>>> +
>>> + process_options(argc, argv);
>>> +
>>> + ent = getpwnam(ltp_user);
>>> + if (ent == NULL) {
>>> + fprintf(stderr, "can't get password entry for %s", ltp_user);
>>> + exit(1);
>>> + }
>>> + ltp_uid = ent->pw_uid;
>>> + ltp_gid = ent->pw_gid;
>>> +
>
> Since you don't wait for fork()'d child processes how about a:
> signal(SIGCHLD, SIG_IGN);
>
> Which will prevent them from ever becoming zombies.
>
Ok, will fix.
>>> + /* Close PEC listening */
>>> + ret = control_pec(sd, &src_addr, PROC_CN_MCAST_IGNORE);
>>> + if (!ret) {
>>> + fprintf(stderr, "failed to close PEC listening\n");
>>> + exit(1);
>>> + }
>>> +
>>> + close(sd);
>>> +
>
> I think you should also flush stdout:
>
> while (fsync(0) == -1) {
> if (errno != EIO)
> break;
> sleep(10); /* retry once every 10 seconds */
> }
>
> Otherwise you've no guarantee that stdout buffers have been flushed
> before the kill (more below). The close() man page has an ominous NOTES
> section which I think suggests what could go wrong.
>
will fix.
>>> +# $1: the test number
>>> +# $2: type of event
>>> +run_case()
>>> +{
>>> + export TST_COUNT=$1
>>> +
>>> + log="$LTPROOT/output/log"
>>> + mkdir $log 2> /dev/null
>>> +
>>> + ./pec_listener > "$log/listener_$1.log" 2>&1 &
>>> + pid=$!
>>> + sleep 1
>
> You might add comments for these sleeps. I guess this one should be
> something like:
>
> # Wait for pec_listener to start listening
>
will fix.
>>> +
>>> + ./event_generator -e $2 > "$log/generator_$1.log" &
>>> +
>>> + wait $!
>
> What does running in the background and then waiting on the
> event_generator ($!) process buy us? Couldn't you just do:
>
> ./event_generator -e $2 > "$log/generator_$1.log"
>
right, don't know what I was thinking when I wrote this.
>>> + ret1=$?
>>> +
>>> + sleep 1
>
> The comment for the above sleep might read:
>
> # Sleep until pec_listener has seen and handled all of the generated
> events
>
> Also, in theory ./event_generator could someday generate multiple events
> (-n <NUM>). Perhaps there ought to be:
>
> NUM_EVENTS=1
>
> at the top and:
>
> sleep $((1*NUM_EVENTS))
>
> here?
>
this makes sense.
> Use of "sleep" in tests worries me because they are generally meant to
> "avoid" races in the underlying test programs but the amount of time to
> sleep makes timing assumptions about the system's ability to run the
> tests pieces.
>
> However, sleep is highly practical while closing all possible races in
> test scripts make the code much more complicated. Hence I personally
> like to see these uses of "sleep" commented.
>
> In this case, depending on the system, load, and the number of events
> generated this sleep may not be sufficient. If there's other load on
> this system or if it's running on a virtual machine then pec_listener
> may not have enough time to process all of the events (from
> event_generator or otherwise) before receiving the signal. Unlikely I
> expect, but possible...
>
I know it's not a good habbit to use "sleep", but it's practical and
commonly used, and the test is not intended to be run in high-load
system. And when we run ltpstress, we will get some failures as
expected, so I think this won't be a problem.
>>> + kill -s SIGINT $pid 2> /dev/null
>
> This worries me a little.
>
> I think we need to ensure that pec_listener has flushed its buffers
> before we kill it -- otherwise sometimes we may get false FAIL results.
> You already have pec_listener catch this signal and exit. The only
> missing part is the fsync().
>
will fix.
>>> + wait $pid
>>> + ret2=$?
>>> +
>>> + if [ $ret1 -ne 0 -o ! -s "$log/generator_$1.log" ]; then
>>> + tst_resm TFAIL "failed to generate process events"
>>> + exit_status=1
>>> + return 1
>>> + fi
>>> +
>>> + if [ $ret2 -ne 0 ]; then
>>> + tst_resm TFAIL "failed to listen process events"
>>> + exit_status=1
>>> + return 1
>>> + fi
>>> +
>>> + event="`cat $log/generator_$1.log`"
>>> + cat "$log/listener_$1.log" | grep "$event" > /dev/null
>>> + if [ $? -eq 0 ]; then
>>> + tst_resm TPASS "get event - $event"
>>> + else
>>> + tst_resm TFAIL "expected event - $event"
>>> + exit_status=1
>>> + fi
>>> +}
>>> +
>>> +run_case 1 "fork"
>>> +run_case 2 "exec"
>>> +run_case 3 "exit"
>>> +run_case 4 "uid"
>>> +run_case 5 "gid"
>>> +
>
> At the top of this script you could put:
>
> EVENT_TEST_CASES=( "fork" "exec" "exit" "uid" "gid" )
> [EMAIL PROTECTED]
>
> Then you could replace the "run_case" invokations with:
>
> i=0
> for CASE in "[EMAIL PROTECTED]" ; do
> run_case $i $CASE
> ((i++))
> done
>
This is better. :)
> I don't see any significant issues -- just a couple obscure problems
> folks aren't very likely to encounter. Personally, I think these tests
> will be a valuable addition to LTP. Thanks for writing them!
>
Thanks for the comments. I'll revise the patch in this week.
> Cheers,
> -Matt Helsley
>
>
>
-------------------------------------------------------------------------
Check out the new SourceForge.net Marketplace.
It's the best place to buy or sell services for
just about anything Open Source.
http://sourceforge.net/services/buy/index.php
_______________________________________________
Ltp-list mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/ltp-list
