Safe way to open files

[L0rdPhi1]

I'm developing a script and want to create a sub that'll open files safely at all times. Below I've posted the sub I'm currently using which seems to be working okay except once in a blue moon a file will go to zero bytes totally randomly with no explanation. I've narrowed the problem down to something in this sub but I can't seem to find what is exactly causing it... so I've come to seek help :) Also, in the below snippet $flock_enabled will either equal 1 or 0 depending on if flock is enabled on the users system or not.

This sub would be used like: my $filehandel = safe_open("some.file"); and the sub fatal_error has nothing more then this in it: my $error = shift;print "content-type: text/html\n\n$error";exit;

sub safe_open {
    my $fn = shift;
    my $fh = do { local *FH };
    my ($mode, $result);
    if ($fn =~ /^>[^>]/) {
        $mode = 1;
    }
    elsif ($fn =~ /^>>/) {
        $mode = 2;
    }

    if ($mode == 1 and $flock_enabled) {
        # Write mode with flock.
        $do_trun = 1;
        $result = sysopen($fh, $fn, O_CREAT | O_WRONLY);    
    }
    elsif ($mode == 1) {
        # Write mode without flock.
        $result = open($fh, ">$fn");
    }
    elsif ($mode == 2) {
        # Append mode.
        $result = open($fh, ">>$fn");
    }
    elsif (!$mode) {
        $result = open($fh, $fn);
    }
    unless ($result) {
        # Open failed.
        fatal_error("Couldn't open $fn.");
    }
    flock($fh, 2) if $flock_enabled;
    truncate($fh, 0) or fatal_error("Could not truncate file $fn.") if
+ $do_trun;
    return $fh;
}

Thanks!

Edit kudra, 2002-06-15 Added READMORE

[vladb]

Althought, I don't have an immediate direct answer to your question, I felt that in the mean time I could suggest a few improvements to your subroutine. I'll update this node if I find a suitable explanation for your problem at hand, though.

Normally I feel it's a good idea to separate the code that reports something from the code that simply does something. Therefore, as an improvement to your sub, I would move the call the fatal_error() method (which simply prints an error string to the client) outside of your subroutine and let the caller script handle the outcome of the safe_open() method. You could also look into making your method die with a custom error message, which could be picked up by a caller script using the try { }; if ($@) { ... } construct. So, the sub would look similar to this:

# MAIN

my $fn = shift;

try {
  safe_open($fn);
};

if ($@) {
  # here, print fatal error
  fatal_error($@);
}

# SUBS
sub safe_open {
    my $fn = shift;
    my $fh = do { local *FH };
    my ($mode, $result);

    # reset the do_trun variable.
    # and also, make it local to this sub.. why keep it global? ;)
    my $do_trun;

    if ($fn =~ /^>[^>]/) {
        $mode = 1;
    }
    elsif ($fn =~ /^>>/) {
        $mode = 2;
    }

    if ($mode == 1 and $flock_enabled) {
        # Write mode with flock.
        $do_trun = 1;
        sysopen($fh, $fn, O_CREAT | O_WRONLY)
           or die "Failed to open '$fn' in write mode with flock.";
    }
    elsif ($mode == 1) {
        # Write mode without flock.
        open($fh, ">$fn")
          or die "Failed to open '$fn' for writing (no flock).";
    }
    elsif ($mode == 2) {
        # Append mode.
        open($fh, ">>$fn")   
         or die "Failed to open '$fn' for appending.";
    }
    elsif (!$mode) {
        open($fh, $fn);
          or die "Failed to open '$fn'."; 
    }

    if ($flock_enabled) {
      flock($fh, 2) or die "Failed to flock '$fn'.";
    }

    if ($do_trun) {
      truncate($fh, 0) or die "Could not truncate file $fn.";
    }

    return $fh;
}



Update: Err.. I guess this is a classical example of how things might go wrong when you resort to using global variables. From the code, I can't quite see where you receive/get/set/modify the $do_trun variable. It might be that you still have it set to some positive (true) value on consequent calls to the method, which causes your file to be truncated. I guess, you'll simply have to reset the $do_trun variable to 0 every time inside the actual sub to eliminate the 'once in a blue moon' phenomena. ;-)

_____________________

# Under Construction

[danger]

Here's an approach that reduces the number of file-opening calls you need to manage --- determine what flags you want on the file handle and use sysopen once. In the following modes 1 and 2 act like > and >> respectively, else the file is opened read-only. This can be altered --- if you'd rather fail on attempting to append to a non-existing file, remove the O_CREAT flag from mode 2 for example.

use Fcntl qw(:DEFAULT :flock);
# assume $flock_enabled set accordingly

sub safe_open {
    my $fn = shift;
    $fn =~ s/^(>>?)//;
    $mode = length($1);
    my $flags = $mode == 1 ? O_CREAT | O_WRONLY | O_TRUNC  :
                $mode == 2 ? O_CREAT | O_WRONLY | O_APPEND :
                O_RDONLY;
    sysopen(my $fh, $fn, $flags) || die "Can't open $fn: $!";
    flock($fh, LOCK_EX) || die "Can't flock $fn: $!" if $flock_enabled
+;
    return $fh;
}

[L0rdPhi1]

I'm now using the below. Anyone see any problems? Should I still use truncate($fh, 0); after seek or does the sysopen take care of that?

sub safe_open {
    my $fn = shift;
    $fn =~ s/^(>>?)//;
    $mode = length($1);
    my $flags = $mode == 1 ? O_CREAT | O_WRONLY | O_TRUNC  :
                $mode == 2 ? O_CREAT | O_WRONLY | O_APPEND :
                O_RDONLY;
    sysopen(my $fh, $fn, $flags) or die "Can't open $fn: $!";
    if ($flock_enabled) {
        flock($fh, LOCK_EX) or die "Can't flock $fn: $!";
    }
    seek $fh, 0, 0;
    return $fh;
}

[Anonymous Monk]

No. You do not want an unconditional seek in the routine. If it was opened with O_TRUNC or O_RDONLY it isn't needed (neither is truncate), if it was opened with O_APPEND then you definitely do not want to seek nor truncate.

[Aristotle]

Update: danger++ made some excellent improvements.

As has already been hinted, at put a my $do_trun; at the top of the routine. use strict and warnings to avoid such problems in the future.

Also, I want to point out something else: if ($fn =~ /^>[^>]/) obviously expects the string to already have the angle brackets embedded. But then you go and add some more in $result = open($fh, ">$fn"); and $result = open($fh, ">>$fn");

And while not detrimental, they shouldn't be in sysopen($fh, $fn, O_CREAT | O_WRONLY); either. Together with the also already suggested removal of your fatal_error calls, I suggest the following:

sub safe_open {
    my $fn = shift;
    # you need not initialize $fh anymore in Perl >= 5.6
    my ($fh, $mode, $result, $do_trun);

    $fn =~ s/^(>>?)//;
    $mode = length($1);

    if ($mode == 1) {
        # write mode
        if($flock_enabled) {
            $do_trun = 1;
            $result = sysopen($fh, $fn, O_CREAT | O_WRONLY);    
        }
        else {
            $result = open($fh, ">$fn");
        }
    }
    elsif ($mode == 2) {
        # append mode
        $result = open($fh, ">>$fn");
    }
    else {
        # read mode
        $result = open($fh, $fn);
    }

    $result or die "Couldn't open $fn: $!";
    flock($fh, 2) or die "Couldn't flock $fn: $!" if $flock_enabled;
    truncate($fh, 0) or die "Could not truncate file $fn: $!" if $do_t
+run;

    return $fh;
}

____________
Makeshifts last the longest.

[L0rdPhi1]

Thanks, That's a good idea :) I just cannot figure out the zero bytes bug... I've managed to stump quite a few people with it, by the way. But I think it has something to do with sysopen and truncate but I can't be sure.


Update:

I totally recoded the old sysopen part but I'm not sure if what I did will work...

sub safe_open {
    my $fn = shift;
    my $fh = do { local *FH };
    my ($mode, $result);
    if ($fn =~ /^>[^>]/) {
        $mode = 1;
    }
    elsif ($fn =~ /^>>/) {
        $mode = 2;
    }
    else {
        $mode = 0;
    }

    if ($mode == 1 and $flock_enabled) {
        # Write mode with flock.
        $truncate_please = 1;
        my $fhtemp = do { local *FHT };
        $result = open( $fhtemp, "+>>$fn" );
        if ($result) {
             $result = open($filehandle, ">&$fhtemp" );
             close($fhtemp);
        }
    }
    elsif ($mode == 1) {
        # Write mode without flock.
        $result = open($fh, ">$fn");
    }
    elsif ($mode == 2) {
        # Append mode.
        $result = open($fh, ">>$fn");
    }
    elsif ($mode == 0) {
        $result = open($fh, $fn);
    }
    unless ($result) {
        # Open failed.
        fatal_error("Couldn't open $fn.");
    }
    flock($fh, 2) if $flock_enabled;
    if ($truncate_please) {
        truncate($fh, 0) or fatal_error("Could not truncate file $fn."
+);
    }
    return $fh;
}

[belg4mit]

Depending on your definition of safe you should be using 3-arg open instead of 2-arg. You could also make your sub take parameters like 3-arg, and save yourself the string bashing ;-)

--
perl -pew "s/\b;([mnst])/'$1/g"