Re: apacheconfig patch review requested

[steve@technolope.org (Steve Stock)]

On Tue, Jan 15, 2002 at 01:35:19PM -0500, Daniel Jacobowitz wrote:
> On Tue, Jan 15, 2002 at 05:27:44PM +0000, Matthew Wilcox wrote:
> > 
> > Hi.  Anyone know perl?  :-)
> > 
> > Bug #12094 (apache's oldest still-open bug!), #59998, #105820 and now
> > #129372 would seem to all be fixable by the patch attached to #105820.
> > Thanks to Steve Stock for supplying the patch.  Since I don't grok perl,
> > I'd just like someone to review it and see if it looks reasonable.
> > If it is, I'll incorporate it in apache 1.3.22-6.
> 
> Well, the two BUG comments upset me a bit.  The latter at least is a
> problem.  It should be possible to solve both fairly easily - the
> former with a hash table (preferably hashing on inode/dev rather than
> filename) and the latter by prepending /etc/apache to relative paths. 
> I'm fairly certain Apache does nothing more complicated than that.

I've fixed up the patch by including an inode lookup to prevent loops
and a simple conversion to absolute filenames using the already existing
$SERVER_ROOT variable.  The updated patch is attached to #105820, I'm
also including it here (is this normally done or is it common practice
to just refer to the bug entry?)

PS: stat isn't used anywhere else so including File::stat doesn't break
    other parts of the program.

--- apacheconfig-1.3.22-5	Sun Dec 16 13:11:58 2001
+++ apacheconfig	Tue Jan 15 23:20:55 2002
@@ -20,6 +20,8 @@
 
 use strict;
 no strict 'vars';
+use IO::File;       # used in read_config
+use File::stat;     # used in read_config
 
 $HTTPD_CONF="/etc/apache/httpd.conf";
 $HTTPD_EX="/usr/share/doc/apache/examples/httpd.conf";
@@ -65,10 +67,20 @@
 
 sub load_config_files ()
 {
+    # Can't use read_config for httpdconf, accessconf, or srmconf as
+    # the contents of these replace the existing config file.  If
+    # we used read_config here all included files would be flattened
+    # into one config file.  This would really upset most people.
+    # Only allconf (and nonvirtconf) use read_config as allconf is
+    # the variable searched for directives when attempting to determine
+    # which modules to activate.
     $main::httpdconf = `cat $HTTPD_CONF`;
     $main::accessconf = `cat $ACCESS_CONF 2> /dev/null`;
     $main::srmconf = `cat $SRM_CONF 2> /dev/null`;
-    $main::allconf = "$httpdconf\n$accessconf\n$srmconf";
+    $main::allconf = join("\n",
+        read_config($HTTPD_CONF),
+        read_config($ACCESS_CONF),
+        read_config($SRM_CONF));
     ($main::nonvirtconf = $allconf) =~ s/\n\s*<Virtual.*VirtualHost>//gs;
     $main::aliasesconf = `cat $ALIASES_CONF 2> /dev/null`;
     $main::servername = loadconf ("ServerName", "localhost");
@@ -96,6 +108,60 @@
     $main::servergroup = "www-data" if $main::servergroup =~ m/-/;
 }
 
+sub read_config {
+    my $file = shift;
+    my $config = '';
+
+    # Convert relative files to absolute based on $SERVER_ROOT.
+    # BUG: doesn't work if actual server root isn't $SERVER_ROOT, however this
+    # case is expected to be highly unlikely for the vast majority of people.
+    if ($file !~ m{^/}) {
+        $file = "$SERVER_ROOT/$file";
+    }
+
+    # Ignore files with inodes that we've already seen,
+    # tip from Daniel Jacobowitz.
+    # Note that our list of seen inodes needs to be persistent
+    # because we are called three times in load_config_files and
+    # we don't want to be hung up by silliness such as both httpd.conf
+    # and srm.conf including the same config file.
+    if (!ref($main::read_config_seen_files) ||
+        ref($main::read_config_seen_files) ne "HASH") {
+        $main::read_config_seen_files = {};
+    }
+    my $filestat = stat($file);
+    if ($filestat) {
+        if ($main::read_config_seen_files->{$filestat->ino}) {
+            # Already read this file, stop here
+            return $config;
+        } else {
+            # Remember this inode
+            $main::read_config_seen_files->{$filestat->ino} = 1;
+        }
+    }
+
+    if (-d $file) {
+        # We have a directory, per apache docs this means that all files in
+        # this directory and below are to be parsed as configuration files.
+        foreach (`find $file -type f -print`) {
+            chomp $_;
+            $config .= read_config($_);
+        }
+    } elsif (-f $file) {
+        # We have a file, store every line except include directives.
+        # For an include replace it with the contents of the file or
+        # set of files (aka directory) it references.
+        my $reader = IO::File->new($file);
+        while (<$reader>) {
+            if (/^\s*include\s+([\S]+)/i) {
+                $config .= read_config($1);
+            } else {
+                $config .= $_;
+            }
+        }
+    }
+    return $config;
+}
 
 sub yorn ($;$)
 {