Developers of Sympa

[Kazuo Moriwaka <address@concealed>]

Hello, 

I attach a patch for locking of /stats, and /config.

This patch also moves a phrase (which create *.lock file) into
a function tools::create_lockfile(). The phrase has a minor bug about
file closing.  It is fixed in create_lockfile() function. 

Thank you.

From: Olivier Salaün - CRU <address@concealed>
Subject: Re: [sympa-dev] Is a lock need for /stats ?
Date: Mon, 04 Jul 2005 17:29:08 +0200

> Thank you for submiting this patch ;
> I suggest you also upload it in our bug/feature tracking system : 
> http://listes.cru.fr/mantis/
> 
> It will probably not be part of Sympa 5.1 release but rather 5.2
> 
> Kazuo Moriwaka wrote:
> 
> >I post NFS-aware locking patch.  Please check attachment.
> >
> >I changed locking module to File::NFSLock. It has shared lock.
> >
> >I'm sorry for I cannot find "include" directive what you wrote.
> >I implement module existence check temporarily with 'eval'.   If there are 
> >some problems, please correct it.

Index: List.pm
===================================================================
--- List.pm     (revision 81)
+++ List.pm     (working copy)
@@ -3880,16 +3880,8 @@
            ## first lock
            if ($include_lock_count == 1) {
                my $lock_file = $self->{'dir'}.'/include.lock';
-               
-               ## Create include.lock if needed
-               unless (-f $lock_file) {
-                   unless (open FH, ">>$lock_file") {
-                       &do_log('err', 'Cannot open %s: %s', $lock_file, $!);
-                       return undef;
-                   }
-               }
-               close $lock_file;
 
+               &tools::create_lockfile($lock_file);
                unless ($list_of_fh{$lock_file} = 
&tools::lock($lock_file,'read')) {
                    return undef;
                }
@@ -4125,15 +4117,8 @@
        my $lock_file = $self->{'dir'}.'/include_admin_user.lock';
        
        ## Create include_admin_user.lock if needed
-       unless (-f $lock_file) {
-           unless (open FH, ">>$lock_file") {
-               &do_log('err', 'Cannot open %s: %s', $lock_file, $!);
-               return undef;
-           }
-       }
+       &tools::create_lockfile($lock_file);
        
-       close $lock_file;
-       
        unless ($list_of_fh{$lock_file} = &tools::lock($lock_file,'read')) {
            return undef;
        }
@@ -4431,15 +4416,7 @@
            my $lock_file = $self->{'dir'}.'/include.lock';
 
            ## Create include.lock if needed
-           unless (-f $lock_file) {
-               unless (open FH, ">>$lock_file") {
-                   &do_log('err', 'Cannot open %s: %s', $lock_file, $!);
-                   return undef;
-               }
-           }
-           close $lock_file;
-           
-
+           &tools::create_lockfile($lock_file);
            unless ($list_of_fh{$lock_file} = 
&tools::lock($lock_file,'read')) {
                return undef;
            }
@@ -6538,32 +6515,39 @@
     my $file = shift;
     do_log('debug3', 'List::_load_stats_file(%s)', $file);
 
-   ## Create the initial stats array.
-   my ($stats, $total, $last_sync, $last_sync_admin_user);
+    ## Create the initial stats array.
+    my ($stats, $total, $last_sync, $last_sync_admin_user);
+    
+    my $lock_file = $file . '.lock';
+
+    ## Create stats.lock if needed 
+    &tools::create_lockfile($lock_file);
+    my $LOCK = &tools::lock($lock_file, 'write') || return undef;
  
-   if (open(L, $file)){     
-       if (<L> =~ 
/^(\d+)\s+(\d+)\s+(\d+)\s+(\d+)(\s+(\d+))?(\s+(\d+))?(\s+(\d+))?/) {
-          $stats = [ $1, $2, $3, $4];
-          $total = $6;
-          $last_sync = $8;
-          $last_sync_admin_user = $10;
-          
-       } else {
-          $stats = [ 0, 0, 0, 0];
-          $total = 0;
-          $last_sync = 0;
-          $last_sync_admin_user = 0;
-       }
-       close(L);
-   } else {
-       $stats = [ 0, 0, 0, 0];
-       $total = 0;
-       $last_sync = 0;
-       $last_sync_admin_user = 0;
-   }
+    if (open(STATS, $file)){     
+       if (<STATS> =~ 
/^(\d+)\s+(\d+)\s+(\d+)\s+(\d+)(\s+(\d+))?(\s+(\d+))?(\s+(\d+))?/) {
+           $stats = [ $1, $2, $3, $4];
+           $total = $6;
+           $last_sync = $8;
+           $last_sync_admin_user = $10;
+           
+       } else {
+           $stats = [ 0, 0, 0, 0];
+           $total = 0;
+           $last_sync = 0;
+           $last_sync_admin_user = 0;
+       }
+       close(STATS);
+    } else {
+       $stats = [ 0, 0, 0, 0];
+       $total = 0;
+       $last_sync = 0;
+       $last_sync_admin_user = 0;
+    }
+    &tools::unlock($lock_file, $LOCK);
 
-   ## Return the array.
-   return ($stats, $total);
+    ## Return the array.
+    return ($stats, $total);
 }
 
 ## Loads the list of subscribers as a tied hash
@@ -8110,10 +8094,7 @@
        ## Get an Exclusive lock
        
        my $lock_file = $self->{'dir'}.'/include_admin_user.lock';
-       unless (open FH, ">>$lock_file") {
-           &do_log('err', 'Cannot open %s: %s', $lock_file, $!);
-           return undef;
-       }
+       &tools::create_lockfile($lock_file);
        unless ($list_of_fh{$lock_file} = &tools::lock($lock_file,'write')) {
            return undef;
        }
@@ -8428,10 +8409,13 @@
     my $last_sync_admin_user = shift;
     
     do_log('debug2', 'List::_save_stats_file(%s, %d, %d, %d)', $file, 
$total,$last_sync,$last_sync_admin_user );
-    
-    open(L, "> $file") || return undef;
-    printf L "%d %.0f %.0f %.0f %d %d %d\n", @{$stats}, $total, $last_sync, 
$last_sync_admin_user;
-    close(L);
+    my $lock_file = $file . '.lock';
+
+    &tools::create_lockfile($lock_file);
+    my $LOCK = &tools::lock($lock_file, 'write') || return undef;
+    open(STATS, "> $file") || &tools::unlock($lock_file, $LOCK) && return 
undef;
+    printf STATS "%d %.0f %.0f %.0f %d %d %d\n", @{$stats}, $total, 
$last_sync, $last_sync_admin_user;
+    &tools::unlock($lock_file, $LOCK);
 }
 
 ## Writes the user list to disk
@@ -9795,6 +9779,7 @@
     do_log('debug3', 'List::_load_admin_file(%s, %s, %s)', $directory, 
$robot, $file);
 
     my $config_file = $directory.'/'.$file;
+    my $lock_file = $config_file . ".lock";
 
     my %admin;
     my (@paragraphs);
@@ -9807,163 +9792,168 @@
        $admin{'defaults'}{$pname} = 1 unless ($::pinfo{$pname}{'internal'});
     }
 
-    unless (open CONFIG, $config_file) {
-       &do_log('info', 'Cannot open %s', $config_file);
-    }
+    &tools::create_lockfile($lock_file);
+    if (my $LOCK = &tools::lock($lock_file, 'read')) {
 
-    ## Split in paragraphs
-    my $i = 0;
-    while (<CONFIG>) {
-       if (/^\s*$/) {
-           $i++ if $paragraphs[$i];
-       }else {
-           push @{$paragraphs[$i]}, $_;
+       unless (open CONFIG, $config_file) {
+           &do_log('info', 'Cannot open %s', $config_file);
        }
-    }
 
-    for my $index (0..$#paragraphs) {
-       my @paragraph = @{$paragraphs[$index]};
+       ## Split in paragraphs
+       my $i = 0;
+       while (<CONFIG>) {
+           if (/^\s*$/) {
+               $i++ if $paragraphs[$i];
+           }else {
+               push @{$paragraphs[$i]}, $_;
+           }
+       }
 
-       my $pname;
+       for my $index (0..$#paragraphs) {
+           my @paragraph = @{$paragraphs[$index]};
 
-       ## Clean paragraph, keep comments
-       for my $i (0..$#paragraph) {
-           my $changed = undef;
-           for my $j (0..$#paragraph) {
-               if ($paragraph[$j] =~ /^\s*\#/) {
-                   chomp($paragraph[$j]);
-                   push @{$admin{'comment'}}, $paragraph[$j];
-                   splice @paragraph, $j, 1;
-                   $changed = 1;
-               }elsif ($paragraph[$j] =~ /^\s*$/) {
-                   splice @paragraph, $j, 1;
-                   $changed = 1;
+           my $pname;
+
+           ## Clean paragraph, keep comments
+           for my $i (0..$#paragraph) {
+               my $changed = undef;
+               for my $j (0..$#paragraph) {
+                   if ($paragraph[$j] =~ /^\s*\#/) {
+                       chomp($paragraph[$j]);
+                       push @{$admin{'comment'}}, $paragraph[$j];
+                       splice @paragraph, $j, 1;
+                       $changed = 1;
+                   }elsif ($paragraph[$j] =~ /^\s*$/) {
+                       splice @paragraph, $j, 1;
+                       $changed = 1;
+                   }
+
+                   last if $changed;
                }
 
-               last if $changed;
+               last unless $changed;
            }
 
-           last unless $changed;
-       }
-
-       ## Empty paragraph
-       next unless ($#paragraph > -1);
-       
-       ## Look for first valid line
-       unless ($paragraph[0] =~ /^\s*([\w-]+)(\s+.*)?$/) {
-           &do_log('info', 'Bad paragraph "%s" in %s', @paragraph, 
$config_file);
-           next;
-       }
+           ## Empty paragraph
+           next unless ($#paragraph > -1);
            
-       $pname = $1;
+           ## Look for first valid line
+           unless ($paragraph[0] =~ /^\s*([\w-]+)(\s+.*)?$/) {
+               &do_log('info', 'Bad paragraph "%s" in %s', @paragraph, 
$config_file);
+               next;
+           }
+           
+           $pname = $1;
 
-       ## Parameter aliases (compatibility concerns)
-       if (defined $alias{$pname}) {
-           $paragraph[0] =~ s/^\s*$pname/$alias{$pname}/;
-           $pname = $alias{$pname};
-       }
-       
-       unless (defined $::pinfo{$pname}) {
-           &do_log('info', 'Unknown parameter "%s" in %s', $pname, 
$config_file);
-           next;
-       }
-
-       ## Uniqueness
-       if (defined $admin{$pname}) {
-           unless (($::pinfo{$pname}{'occurrence'} eq '0-n') or
-                   ($::pinfo{$pname}{'occurrence'} eq '1-n')) {
-               &do_log('info', 'Multiple parameter "%s" in %s', $pname, 
$config_file);
+           ## Parameter aliases (compatibility concerns)
+           if (defined $alias{$pname}) {
+               $paragraph[0] =~ s/^\s*$pname/$alias{$pname}/;
+               $pname = $alias{$pname};
            }
-       }
-       
-       ## Line or Paragraph
-       if (ref $::pinfo{$pname}{'file_format'} eq 'HASH') {
-           ## This should be a paragraph
-           unless ($#paragraph > 0) {
-               &do_log('info', 'Expecting a paragraph for "%s" parameter in 
%s', $pname, $config_file);
-           }
            
-           ## Skipping first line
-           shift @paragraph;
+           unless (defined $::pinfo{$pname}) {
+               &do_log('info', 'Unknown parameter "%s" in %s', $pname, 
$config_file);
+               next;
+           }
 
-           my %hash;
-           for my $i (0..$#paragraph) {            
-               next if ($paragraph[$i] =~ /^\s*\#/);
-               
-               unless ($paragraph[$i] =~ /^\s*(\w+)\s*/) {
-                   &do_log('info', 'Bad line "%s" in %s',$paragraph[$i], 
$config_file);
+           ## Uniqueness
+           if (defined $admin{$pname}) {
+               unless (($::pinfo{$pname}{'occurrence'} eq '0-n') or
+                       ($::pinfo{$pname}{'occurrence'} eq '1-n')) {
+                   &do_log('info', 'Multiple parameter "%s" in %s', $pname, 
$config_file);
                }
-               
-               my $key = $1;
-               
-               unless (defined $::pinfo{$pname}{'file_format'}{$key}) {
-                   &do_log('info', 'Unknown key "%s" in paragraph "%s" in 
%s', $key, $pname, $config_file);
-                   next;
+           }
+           
+           ## Line or Paragraph
+           if (ref $::pinfo{$pname}{'file_format'} eq 'HASH') {
+               ## This should be a paragraph
+               unless ($#paragraph > 0) {
+                   &do_log('info', 'Expecting a paragraph for "%s" parameter 
in %s', $pname, $config_file);
                }
                
-               unless ($paragraph[$i] =~ 
/^\s*$key\s+($::pinfo{$pname}{'file_format'}{$key}{'file_format'})\s*$/i) {
-                   &do_log('info', 'Bad entry "%s" in paragraph "%s" in %s', 
$paragraph[$i], $key, $pname, $config_file);
-                   next;
-               }
+               ## Skipping first line
+               shift @paragraph;
 
-               $hash{$key} = &_load_list_param($robot,$key, $1, 
$::pinfo{$pname}{'file_format'}{$key}, $directory);
-           }
-
-           ## Apply defaults & Check required keys
-           my $missing_required_field;
-           foreach my $k (keys %{$::pinfo{$pname}{'file_format'}}) {
-
-               ## Default value
-               unless (defined $hash{$k}) {
-                   if (defined 
$::pinfo{$pname}{'file_format'}{$k}{'default'}) {
-                       $hash{$k} = &_load_list_param($robot,$k, 'default', 
$::pinfo{$pname}{'file_format'}{$k}, $directory);
+               my %hash;
+               for my $i (0..$#paragraph) {        
+                   next if ($paragraph[$i] =~ /^\s*\#/);
+                   
+                   unless ($paragraph[$i] =~ /^\s*(\w+)\s*/) {
+                       &do_log('info', 'Bad line "%s" in %s',$paragraph[$i], 
$config_file);
                    }
+                   
+                   my $key = $1;
+                   
+                   unless (defined $::pinfo{$pname}{'file_format'}{$key}) {
+                       &do_log('info', 'Unknown key "%s" in paragraph "%s" 
in %s', $key, $pname, $config_file);
+                       next;
+                   }
+                   
+                   unless ($paragraph[$i] =~ 
/^\s*$key\s+($::pinfo{$pname}{'file_format'}{$key}{'file_format'})\s*$/i) {
+                       &do_log('info', 'Bad entry "%s" in paragraph "%s" in 
%s', $paragraph[$i], $key, $pname, $config_file);
+                       next;
+                   }
+
+                   $hash{$key} = &_load_list_param($robot,$key, $1, 
$::pinfo{$pname}{'file_format'}{$key}, $directory);
                }
 
-               ## Required fields
-               if ($::pinfo{$pname}{'file_format'}{$k}{'occurrence'} eq '1') 
{
+               ## Apply defaults & Check required keys
+               my $missing_required_field;
+               foreach my $k (keys %{$::pinfo{$pname}{'file_format'}}) {
+
+                   ## Default value
                    unless (defined $hash{$k}) {
-                       &do_log('info', 'Missing key "%s" in param "%s" in 
%s', $k, $pname, $config_file);
-                       $missing_required_field++;
+                       if (defined 
$::pinfo{$pname}{'file_format'}{$k}{'default'}) {
+                           $hash{$k} = &_load_list_param($robot,$k, 
'default', $::pinfo{$pname}{'file_format'}{$k}, $directory);
+                       }
                    }
+
+                   ## Required fields
+                   if ($::pinfo{$pname}{'file_format'}{$k}{'occurrence'} eq 
'1') {
+                       unless (defined $hash{$k}) {
+                           &do_log('info', 'Missing key "%s" in param "%s" 
in %s', $k, $pname, $config_file);
+                           $missing_required_field++;
+                       }
+                   }
                }
-           }
 
-           next if $missing_required_field;
+               next if $missing_required_field;
 
-           delete $admin{'defaults'}{$pname};
+               delete $admin{'defaults'}{$pname};
 
-           ## Should we store it in an array
-           if (($::pinfo{$pname}{'occurrence'} =~ /n$/)) {
-               push @{$admin{$pname}}, \%hash;
+               ## Should we store it in an array
+               if (($::pinfo{$pname}{'occurrence'} =~ /n$/)) {
+                   push @{$admin{$pname}}, \%hash;
+               }else {
+                   $admin{$pname} = \%hash;
+               }
            }else {
-               $admin{$pname} = \%hash;
-           }
-       }else {
-           ## This should be a single line
-           unless ($#paragraph == 0) {
-               &do_log('info', 'Expecting a single line for "%s" parameter 
in %s', $pname, $config_file);
-           }
+               ## This should be a single line
+               unless ($#paragraph == 0) {
+                   &do_log('info', 'Expecting a single line for "%s" 
parameter in %s', $pname, $config_file);
+               }
 
-           unless ($paragraph[0] =~ 
/^\s*$pname\s+($::pinfo{$pname}{'file_format'})\s*$/i) {
-               &do_log('info', 'Bad entry "%s" in %s', $paragraph[0], 
$config_file);
-               next;
-           }
+               unless ($paragraph[0] =~ 
/^\s*$pname\s+($::pinfo{$pname}{'file_format'})\s*$/i) {
+                   &do_log('info', 'Bad entry "%s" in %s', $paragraph[0], 
$config_file);
+                   next;
+               }
 
-           my $value = &_load_list_param($robot,$pname, $1, 
$::pinfo{$pname}, $directory);
+               my $value = &_load_list_param($robot,$pname, $1, 
$::pinfo{$pname}, $directory);
 
-           delete $admin{'defaults'}{$pname};
+               delete $admin{'defaults'}{$pname};
 
-           if (($::pinfo{$pname}{'occurrence'} =~ /n$/)
-               && ! (ref ($value) =~ /^ARRAY/)) {
-               push @{$admin{$pname}}, $value;
-           }else {
-               $admin{$pname} = $value;
+               if (($::pinfo{$pname}{'occurrence'} =~ /n$/)
+                   && ! (ref ($value) =~ /^ARRAY/)) {
+                   push @{$admin{$pname}}, $value;
+               }else {
+                   $admin{$pname} = $value;
+               }
            }
        }
+       
+       close CONFIG;
+       &tools::unlock($lock_file, $LOCK);
     }
-    
-    close CONFIG;
 
     ## Apply defaults & check required parameters
     foreach my $p (keys %::pinfo) {
@@ -10096,8 +10086,12 @@
 ## Save a config file
 sub _save_admin_file {
     my ($config_file, $old_config_file, $admin) = @_;
+    my $lock_file = $config_file . '.lock';
     do_log('debug3', 'List::_save_admin_file(%s, %s, %s)', 
$config_file,$old_config_file, $admin);
 
+    &tools::create_lockfile($lock_file);
+    my $LOCK = &tools::lock($lock_file, 'write') || return undef;
+
     unless (rename $config_file, $old_config_file) {
        &do_log('notice', 'Cannot rename %s to %s', $config_file, 
$old_config_file);
        return undef;
@@ -10130,6 +10124,7 @@
 
     }
     close CONFIG;
+    &tools::unlock($lock_file, $LOCK);
 
     return 1;
 }
Index: tools.pl
===================================================================
--- tools.pl    (revision 81)
+++ tools.pl    (working copy)
@@ -1925,7 +1925,19 @@
 sub LOCK_UN {8};
 
 
+sub create_lockfile {
+    my $lock_file = shift;
 
+    ## Create lockfile if needed
+    unless (-f $lock_file) {
+       unless (open FH, ">>$lock_file") {
+           &do_log('err', 'Cannot open %s: %s', $lock_file, $!);
+           return undef;
+       }
+       close FH;
+    }
+}
+
 ## lock a file 
 sub lock {
     my $lock_file = shift;