Im I forking properly ?

[leostereo]

This is the first time I need to do fork , so Im reading and trying to learn how to do it.
Im reading and processing the log lines of my dhcp sever wich are coming really fast.
For each line I receive I need to perform a snmpget querie to the ips Im grabing.
The problem is that each querie takes more than a second to return a value so it is delaying all the script so I had to do fork.
I have this first version working but I would like have your opinion in order to improve it or make it run in a more secure way.

#!/usr/bin/perl -w
use strict;
use File::Tail;
use Cwd qw();
my $path = Cwd::cwd();

$SIG{'CHLD'} = "wait_for_child_to_die";

my $community='public';    
my $snmp_fver = '.1.3.6.1.4.1.2700.1.1.8.0';

my $file = File::Tail->new(
        name =>'/var/log/messages', 
        interval => 1, 
        maxinterval => 1, 
#        resetafter=> 5, 
        );

while (defined(my $line=$file->read)) {
         #print $line,"\n";
 
                if ($line =~ /^(.*) dns02cor dhcpd: DHCPACK on ([0-9\.
+]+) to ([[:xdigit:]:]+).* ([0-9\.]+)/) {
                unless (my $pid = fork) {
                die "Couldn't fork " unless defined $pid;
             
                write_register($1,$2,$3);  
                exit;
                }    
            }
}

sub write_register {
my ($date,$client_ip,$client_imsi) = @_;

 $output=qx(snmpwalk -v2c -t1 -c $community $client_ip $snmp_fver 2>&1
+);
 chomp($output);
        if( $output eq "Timeout: No Response from $client_ip" ) {
        return; 
        }
        else{
                   my @result=split(/:/,$output);
                   if ($result[3]){
           $fver=$result[3];
                   $fver=~s/ //g;
                   $fver=~s/\n//g;

                   }else{
            exit;
            }
               }

        if($bsid){
                system "sed -i '/$client_imsi/d' $path/leases_list.txt
+";
        system 'echo "'.$date.','.$client_ip.','.$client_imsi.','. $fv
+er.'" >> '.$path.'/leases_list.txt';
        }
        
}
sub wait_for_child_to_die
{
    wait;
}
1;

I must admint this is a mix of multiple scripts I founded on the web.
I hope to hear your opinion in case is there is something I should be aware or change.
I promess to read and learn about forking , wait , and related topics.
So far , it is not creating more than 8 process and no zombies also so Im very happy.
Regards,
Leandro.

[flexvault]

leostereo,

Since this works for you, everything may be Great!

However since this process is long-running, it might be better to pre-fork some number (maybe 8) of waiting children. Currently, you 'fork' for each message, and 'fork' uses a lot of computer power just to start.

If you 'Super Search' on 'pre-fork' you should find examples.

Use 'kill -0 $pid' to see if the child is *working*, and the parent can 'fork' another child is not.
Please note, this is not a trivial script, but once you learn how to use the technique, you'll be very glad/proud of your work!

Good Luck

Regards...Ed

[NetWallah]

You have encountered a fairly common design pattern, where you have a relatively quick "work supplier" in "tail"in the file, and a relatively slow "consumer" process, which does the snmpget.

The usual solution to this is to run multiple, parallel "consumer" processes, such that, on the average, consumption rate matches generation rate.

One solution to this uses threads to run worker processes, and queues to communicate with them.

This site contains many such queries, and implementations of this pattern, this one being the most recent. Please review the techniques, and I recommend following BrowserUk's brilliant responses.

[leostereo]

Thanks!! I will read those pappers as you suggested

[Anonymous Monk]

Your script is horrible to read, it's prone to creating zombie processes and it doesn't even compile.

So, no, you're not doing it properly.

I was hoping that some other monk would tell you that more politely, but no one did, so I'm just telling you like it is.

As for what is wrong: $output and $fver are not declared anywhere, so use strict aborts compilation. wait in a SIGCHLD handler is just wrong, because it waits only for one child, but:

... when the signal is delivered to the process by the system (to the C code that implements Perl) a flag is set, and the handler returns immediately.
    Then at strategic "safe" points in the Perl interpreter (e.g. when it is about to execute a new opcode) the flags are checked and the Perl level handler from %SIG is executed.
...
        As the Perl interpreter looks at signal flags only when it is about to execute a new opcode, a signal that arrives during a long-running opcode (e.g. a regular expression operation on a very large string)  will not be seen until the current opcode completes.
        If a signal of any given type fires multiple times during an opcode  (such as from a fine-grained timer), the handler for that signal will be called only once, after the opcode completes; all other instances will be discarded.

In addition, the flag-setting handler in perl blocks SIGCHLD signal while it's executing; and only one blocking signal can be pending, so, if two or more children die during that, you'll get at least one zombie.

Recommended fix:

$SIG{CHLD} = 'IGNORE';

Recommended reading:

perldoc perlstyle
perldoc strict
perldoc perlipc
man 7 signal
man 2 wait
man 2 sigaction
perldoc -f wait
perldoc -v '%SIG'

Also, see "The Linux Programming Interface", chapters 20-22 (applicable to all Unix-like systems, not only Linux).

[leostereo]

I really apreciate your honesty!!! and thanks for the suggested read material!

[leostereo]

just one thing ... Why is it so horrible to read??

[Athanasius]

Hello leostereo,

Why is it so horrible to read??

Because the haphazard indentation makes it almost impossible to tell, at a glance, where each logical block of code ends. Here’s the original version of sub write_register, with a single comment added:

Read more... (1253 Bytes)

Now, quickly: which block of code is terminated by the right brace named X?

Here’s a preliminary clean-up of the code, with consistent indentation:

Read more... (1263 Bytes)

Easier to follow now? Yes, and that makes it easier to spot where it can be improved:

1. As noted by Anonymous Monk++, the variables $output and $fver are never declared. In addition, the variable $bsid is neither declared nor initialized anywhere in the script.
2. Perl allows you to return, exit, or die at any point in the code. Since these statements are forms of flow control, the code can often be simplified when these statements are used. For example, this:

   if (condition)
   {
       return;
   }
   else
   {
       ...
   }
   

   is logically equivalent to the shorter and simpler:

   return if condition;
   ...
   

3. It’s bad practice to exit from a subroutine. You should die (i.e., throw an exception) instead: that way, the subroutine’s clients have the option to either handle the exception locally or allow it to propagate upwards and so terminate the script.

Here’s an improved version of the subroutine:

Read more... (1108 Bytes)

As a rule, the shorter and simpler the code, the easier it is to debug and maintain. Disclaimer: I haven’t looked at the code’s logic, only its form.

Hope that helps,