Significantly reworking the internals of the fileparsing code. It now

passes around an instance of a FileRecord, rather than just a hash, which I
think makes it much easier to understand.

Moved the sshkey parsed provider test to its own directory and made it better.

This work is all being done so I can move cron jobs to using providers
instead of the current unmaintainable state of affairs.



git-svn-id: https://reductivelabs.com/svn/puppet/trunk@2281 980ebf18-57e1-0310-9a29-db15c13687c0

7 files changed, 285 insertions, 547 deletions

diff --git a/lib/puppet/provider.rb b/lib/puppet/provider.rb
index 3a31a89a3..23c921ac0 100644
--- a/lib/puppet/provider.rb
+++ b/lib/puppet/provider.rb
@@ -123,6 +123,22 @@ class Puppet::Provider
         end
     end
 
+    # Create getter/setter methods for each property our model supports.
+    # They all get stored in @property_hash.  This method is useful
+    # for those providers that use prefetch and flush.
+    def self.mkmodelmethods
+        [model.validproperties, model.parameters].flatten.each do |attr|
+            attr = symbolize(attr)
+            define_method(attr) do
+                @property_hash[attr] || :absent
+            end
+
+            define_method(attr.to_s + "=") do |val|
+                @property_hash[attr] = val
+            end
+        end
+    end
+
     self.initvars
 
     # Check whether this implementation is suitable for our platform.
@@ -208,6 +224,7 @@ class Puppet::Provider
 
     def initialize(model)
         @model = model
+        @property_hash = {}
     end
 
     def name
diff --git a/lib/puppet/provider/cron/crontab.rb b/lib/puppet/provider/cron/crontab.rb
new file mode 100755
index 000000000..c50bc2680
--- /dev/null
+++ b/lib/puppet/provider/cron/crontab.rb
@@ -0,0 +1,90 @@
+require 'puppet/provider/parsedfile'
+
+tab = case Facter.value(:operatingsystem)
+    when "Solaris": :suntab
+    else
+        :crontab
+    end
+
+Puppet::Type.type(:cron).provide(:crontab,
+    :parent => Puppet::Provider::ParsedFile,
+    :filetype => tab
+) do
+    commands :crontab => "crontab"
+
+    attr_accessor :target
+
+    text_line :comment, :match => %r{^#}, :post_parse => proc { |hash|
+        if hash[:line] =~ /Puppet Name: (.+)\s*$/
+            hash[:name] = $1
+        end
+    }
+
+    text_line :blank, :match => /^\s+/
+
+    text_line :environment, :match => %r{/^\w+=/}
+
+    record_line :crontab, :fields => %w{minute hour weekday month monthday command},
+        :optional => %w{minute hour weekday month monthday}, :absent => "*"
+
+    #record_line :freebsd_special, :fields => %w{special command},
+    #    :match => %r{^@(\w+)\s+(.+)}
+    # Apparently Freebsd will "helpfully" add a new TZ line to every
+    # single cron line, but not in all cases (e.g., it doesn't do it
+    # on my machine.  This is my attempt to fix it so the TZ lines don't
+    # multiply.
+    #if tab =~ /^TZ=.+$/
+    #    return tab.sub(/\n/, "\n" + self.header)
+    #else
+    #    return self.header() + tab
+    #end
+
+
+    # Return the header placed at the top of each generated file, warning
+    # users that modifying this file manually is probably a bad idea.
+    def self.header
+%{# HEADER This file was autogenerated at #{Time.now} by puppet.  While it
+# HEADER can still be managed manually, it is definitely not recommended.
+# HEADER Note particularly that the comments starting with 'Puppet Name' should
+# HEADER not be deleted, as doing so could cause duplicate cron jobs.\n}
+    end
+
+    # Collapse name and env records.
+    def self.prefetch_hook(records)
+        name = nil
+        envs = []
+        records.each { |record|
+            case record[:record_type]
+            when :comment:
+                if record[:name]
+                    name = record[:name]
+                    record[:skip] = true
+                end
+            when :environment:
+                if name
+                    envs << record[:line]
+                    record[:skip] = true
+                end
+            else
+                if name
+                    record[:name] = name
+                    name = nil
+                end
+                unless envs.empty?
+                    record[:environment] = envs
+                    envs = []
+                end
+            end
+        }.reject { |record| record[:skip] }
+    end
+
+    def user=(user)
+        self.target = target
+    end
+
+    def user
+        self.target
+    end
+end
+
+# $Id$
diff --git a/lib/puppet/provider/parsedfile.rb b/lib/puppet/provider/parsedfile.rb
index f1b77e56b..56474e273 100755
--- a/lib/puppet/provider/parsedfile.rb
+++ b/lib/puppet/provider/parsedfile.rb
@@ -117,8 +117,8 @@ class Puppet::Provider::ParsedFile < Puppet::Provider
         list.collect { |r| r[:name] }
     end
 
-    # Create attribute methods for each of the model's non-metaparam attributes.
-    def self.model=(model)
+    # Override the default method with a lot more functionality.
+    def self.mkmodelmethods
         [model.validproperties, model.parameters].flatten.each do |attr|
             attr = symbolize(attr)
             define_method(attr) do
@@ -152,7 +152,12 @@ class Puppet::Provider::ParsedFile < Puppet::Provider
                 @property_hash[attr] = val
             end
         end
-        @model = model
+    end
+
+    # Always make the model methods.
+    def self.model=(model)
+        super
+        mkmodelmethods()
     end
 
     # Mark a target as modified so we know to flush it.  This only gets
@@ -179,12 +184,18 @@ class Puppet::Provider::ParsedFile < Puppet::Provider
 
     # Prefetch an individual target.
     def self.prefetch_target(target)
-        @records += retrieve(target).each do |r|
+        target_records = retrieve(target).each do |r|
             r[:on_disk] = true
             r[:target] = target
             r[:ensure] = :present
         end
 
+        if respond_to?(:prefetch_hook)
+            prefetch_hook(target_records)
+        end
+
+        @records += target_records
+
         # Set current property on any existing resource instances.
         target_records(target).find_all { |i| i.is_a?(Hash) }.each do |record|
             # Find any model instances whose names match our instances.
@@ -222,7 +233,6 @@ class Puppet::Provider::ParsedFile < Puppet::Provider
             ensure
                 @target = old
             end
-
         end
     end
 
diff --git a/lib/puppet/provider/sshkey/parsed.rb b/lib/puppet/provider/sshkey/parsed.rb
index e247502ff..d6e175cf8 100755
--- a/lib/puppet/provider/sshkey/parsed.rb
+++ b/lib/puppet/provider/sshkey/parsed.rb
@@ -1,6 +1,5 @@
 require 'puppet/provider/parsedfile'
 
-
 known = nil
 case Facter.value(:operatingsystem)
 when "Darwin": known = "/etc/ssh_known_hosts"
@@ -15,30 +14,23 @@ Puppet::Type.type(:sshkey).provide(:parsed,
 ) do
     text_line :comment, :match => /^#/
     text_line :blank, :match => /^\s+/
-    record_line :parsed, :fields => %w{name type key}
-    
-    # Override the line parsing a bit, so we can split the aliases out.
-    def self.parse_line(line)
-        hash = super
-        if hash[:name] =~ /,/
-            names = hash[:name].split(",")
-            hash[:name] = names.shift
-            hash[:alias] = names
-        end
-        hash
-    end
-        
-    
-    def self.to_line(hash)
-        if hash[:alias]
-            hash = hash.dup
-            names = [hash[:name], hash[:alias]].flatten
-            
-            hash[:name] = [hash[:name], hash[:alias]].flatten.join(",")
-            hash.delete(:alias)
-        end
-        super(hash)
-    end
+
+    record_line :parsed, :fields => %w{name type key},
+        :post_parse => proc { |hash|
+            if hash[:name] =~ /,/
+                names = hash[:name].split(",")
+                hash[:name] = names.shift
+                hash[:alias] = names
+            end
+        },
+        :pre_gen => proc { |hash|
+            if hash[:alias]
+                names = [hash[:name], hash[:alias]].flatten
+                
+                hash[:name] = [hash[:name], hash[:alias]].flatten.join(",")
+                hash.delete(:alias)
+            end
+        }
 end
 
 # $Id$
diff --git a/lib/puppet/type/cron.rb b/lib/puppet/type/cron.rb
index a91af13ed..d71654586 100755
--- a/lib/puppet/type/cron.rb
+++ b/lib/puppet/type/cron.rb
@@ -11,75 +11,11 @@ module Puppet
     # and is used to manage the job.
     newtype(:cron) do
 
-        # A stupid hack because Cron is the only parsed type that I haven't
-        # converted to using providers.  This whole stupid type needs to
-        # be rewritten.
-        class CronHackParam < Puppet::Property::ParsedParam
-            # Normally this would retrieve the current value, but our property is not
-            # actually capable of doing so.
-            def retrieve
-                # If we've synced, then just copy the values over and return.
-                # This allows this property to behave like any other property.
-                if defined? @synced and @synced
-                    # by default, we only copy over the first value.
-                    @is = @synced
-                    @synced = false
-                    return
-                end
-
-                unless defined? @is and ! @is.nil?
-                    @is = :absent
-                end
-            end
-
-            # If the ensure property is out of sync, it will always be called
-            # first, so I don't need to worry about that.
-            def sync(nostore = false)
-                ebase = @parent.class.name.to_s
-
-                tail = nil
-                if self.class.name == :ensure
-                    # We're either creating or destroying the object
-                    if @is == :absent
-                        #@is = self.should
-                        tail = "created"
-
-                        # If we're creating it, then sync all of the other properties
-                        # but tell them not to store (we'll store just once,
-                        # at the end).
-                        unless nostore
-                            @parent.eachproperty { |property|
-                                next if property == self or property.name == :ensure
-                                property.sync(true)
-                            }
-                        end
-                    elsif self.should == :absent
-                        @parent.remove(true)
-                        tail = "deleted"
-                    end
-                else
-                    # We don't do the work here, it gets done in 'store'
-                    tail = "changed"
-                end
-                @synced = self.should
-
-                # This should really only be done once per run, rather than
-                # every time.  I guess we need some kind of 'flush' mechanism.
-                if nostore
-                    self.retrieve
-                else
-                    @parent.store
-                end
-                
-                return (ebase + "_" + tail).intern
-            end
-        end
-
         # A base class for all of the Cron parameters, since they all have
         # similar argument checking going on.  We're stealing the base class
         # from parsedtype, and we should probably subclass Cron from there,
         # but it was just too annoying to do.
-        class CronParam < CronHackParam
+        class CronParam < Puppet::Property
             class << self
                 attr_accessor :boundaries, :default
             end
@@ -215,14 +151,6 @@ module Puppet
             end
         end
 
-
-        # Override 'newproperty' so that all properties default to having the
-        # correct parent type
-        def self.newproperty(name, options = {}, &block)
-            options[:parent] ||= Puppet::Property::CronParam
-            super(name, options, &block)
-        end
-
         # Somewhat uniquely, this property does not actually change anything -- it
         # just calls +@parent.sync+, which writes out the whole cron tab for
         # the user in question.  There is no real way to change individual cron
@@ -253,7 +181,7 @@ module Puppet
             end
         end
 
-        newproperty(:special, :parent => CronHackParam) do
+        newproperty(:special) do
             desc "Special schedules only supported on FreeBSD."
 
             def specials
@@ -268,19 +196,19 @@ module Puppet
             end
         end
 
-        newproperty(:minute) do
+        newproperty(:minute, :parent => CronParam) do
             self.boundaries = [0, 59]
             desc "The minute at which to run the cron job.
                 Optional; if specified, must be between 0 and 59, inclusive."
         end
 
-        newproperty(:hour) do
+        newproperty(:hour, :parent => CronParam) do
             self.boundaries = [0, 23]
             desc "The hour at which to run the cron job. Optional;
                 if specified, must be between 0 and 23, inclusive."
         end
 
-        newproperty(:weekday) do
+        newproperty(:weekday, :parent => CronParam) do
             def alpha
                 %w{sunday monday tuesday wednesday thursday friday saturday}
             end
@@ -290,7 +218,7 @@ module Puppet
                 0 being Sunday, or must be the name of the day (e.g., Tuesday)."
         end
 
-        newproperty(:month) do
+        newproperty(:month, :parent => CronParam) do
             def alpha
                 %w{january february march april may june july
                     august september october november december}
@@ -300,13 +228,13 @@ module Puppet
                 must be between 1 and 12 or the month name (e.g., December)."
         end
 
-        newproperty(:monthday) do
+        newproperty(:monthday, :parent => CronParam) do
             self.boundaries = [1, 31]
             desc "The day of the month on which to run the
                 command.  Optional; if specified, must be between 1 and 31."
         end
 
-        newproperty(:environment, :parent => CronHackParam) do
+        newproperty(:environment) do
             desc "Any environment settings associated with this cron job.  They
                 will be stored between the header and the job in the crontab.  There
                 can be no guarantees that other, earlier settings will not also
@@ -364,7 +292,7 @@ module Puppet
             end
         end
 
-        newparam(:user) do
+        newproperty(:user) do
             desc "The user to run the command as.  This user must
                 be allowed to run cron jobs, which is not currently checked by
                 Puppet.
@@ -372,15 +300,6 @@ module Puppet
                 The user defaults to whomever Puppet is running as."
 
             defaultto { ENV["USER"] }
-
-            def value=(value)
-                super
-
-                # Make sure the user is not an array
-                if @value.is_a? Array
-                    @value = @value[0]
-                end
-            end
         end
 
         @doc = "Installs and manages cron jobs.  All fields except the command 
@@ -405,58 +324,8 @@ module Puppet
                 }
             "
 
-        @instances = {}
-        @tabs = {}
-
-        class << self
-            attr_accessor :filetype
-
-            def cronobj(name)
-                if defined? @tabs
-                    return @tabs[name]
-                else
-                    return nil
-                end
-            end
-        end
-
         attr_accessor :uid
 
-        # In addition to removing the instances in @objects, Cron has to remove
-        # per-user cron tab information.
-        def self.clear
-            @instances = {}
-            @tabs = {}
-            super
-        end
-
-        def self.defaulttype
-            case Facter["operatingsystem"].value
-            when "Solaris":
-                return Puppet::Util::FileType.filetype(:suntab)
-            else
-                return Puppet::Util::FileType.filetype(:crontab)
-            end
-        end
-
-        self.filetype = self.defaulttype()
-
-        # Override the default Puppet::Type method, because instances
-        # also need to be deleted from the @instances hash
-        def self.delete(child)
-            if @instances.include?(child[:user])
-                if @instances[child[:user]].include?(child)
-                    @instances[child[:user]].delete(child)
-                end
-            end
-            super
-        end
-
-        # Return the fields found in the cron tab.
-        def self.fields
-            return [:minute, :hour, :monthday, :month, :weekday, :command]
-        end
-
         # Convert our hash to an object
         def self.hash2obj(hash)
             obj = nil
@@ -506,33 +375,6 @@ module Puppet
             instance(obj)
         end
 
-        # Return the header placed at the top of each generated file, warning
-        # users that modifying this file manually is probably a bad idea.
-        def self.header
-%{# HEADER This file was autogenerated at #{Time.now} by puppet.  While it
-# HEADER can still be managed manually, it is definitely not recommended.
-# HEADER Note particularly that the comments starting with 'Puppet Name' should
-# HEADER not be deleted, as doing so could cause duplicate cron jobs.\n}
-        end
-
-        def self.instance(obj)
-            user = obj[:user]
-            unless @instances.include?(user)
-                @instances[user] = []
-            end
-
-            @instances[user] << obj
-        end
-
-        def self.list
-            # Look for cron jobs for each user
-            Puppet::Type.type(:user).list_by_name.each { |user|
-                self.retrieve(user, false)
-            }
-
-            self.collect { |c| c }
-        end
-
         # See if we can match the hash against an existing cron job.
         def self.match(hash)
             self.find_all { |obj|
@@ -580,276 +422,6 @@ module Puppet
             return false
         end
 
-        # Parse a user's cron job into individual cron objects.
-        #
-        # Autogenerates names for any jobs that don't already have one; these
-        # names will get written back to the file.
-        #
-        # This method also stores existing comments, and it stores all cron
-        # jobs in order, mostly so that comments are retained in the order
-        # they were written and in proximity to the same jobs.
-        def self.parse(user, text)
-            count = 0
-            hash = {}
-
-            envs = []
-            text.chomp.split("\n").each { |line|
-                case line
-                when /^# Puppet Name: (.+)$/
-                    hash[:name] = $1
-                    next
-                when /^#/:
-                    # add other comments to the list as they are
-                    @instances[user] << line 
-                    next
-                when /^\s*(\w+)\s*=\s*(.+)\s*$/:
-                    # Match env settings.
-                    if hash[:name]
-                        envs << line
-                    else
-                        @instances[user] << line 
-                    end
-                    next
-                when /^@(\w+)\s+(.+)/ # FreeBSD special cron crap
-                    fields().each do |field|
-                        next if field == :command
-                        hash[field] = :absent
-                    end
-                    hash[:special] = $1
-                    hash[:command] = $2
-                else
-                    if match = /^(\S+) (\S+) (\S+) (\S+) (\S+) (.+)$/.match(line)
-                        fields().zip(match.captures).each { |param, value|
-                            if value == "*"
-                                hash[param] = [:absent]
-                            else
-                                if param == :command
-                                    hash[param] = [value]
-                                else
-                                    # We always want the 'is' value to be an
-                                    # array
-                                    hash[param] = value.split(",")
-                                end
-                            end
-                        }
-                    else
-                        # Don't fail on unmatched lines, just warn on them
-                        # and skip them.
-                        Puppet.warning "Could not match '%s'" % line
-                        next
-                    end
-                end
-
-                unless envs.empty?
-                    # We have to dup here so that we don't remove the settings
-                    # in @is on the object.
-                    hash[:environment] = envs.dup
-                end
-
-                hash[:user] = user
-
-                # Now convert our hash to an object.
-                hash2obj(hash)
-
-                hash = {}
-                envs.clear
-                count += 1
-            }
-        end
-
-        # Retrieve a given user's cron job, using the @filetype's +retrieve+
-        # method.  Returns nil if there was no cron job; else, returns the
-        # number of cron instances found.
-        def self.retrieve(user, checkuser = true)
-            # First make sure the user exists, unless told not to
-            if checkuser
-                begin
-                    Puppet::Util.uid(user)
-                rescue ArgumentError
-                    raise Puppet::Error,  "User %s not found" % user
-                end
-            end
-
-            @tabs[user] ||= @filetype.new(user)
-            text = @tabs[user].read
-            if $? != 0
-                # there is no cron file
-                return nil
-            else
-                # Preemptively mark everything absent, so that retrieving it
-                # can mark it present again.
-                self.find_all { |obj|
-                    obj[:user] == user
-                }.each { |obj|
-                    obj.is = [:ensure, :absent]
-                }
-
-                # Get rid of the old instances, so we don't get duplicates
-                if @instances.include?(user)
-                    @instances[user].clear
-                else
-                    @instances[user] = []
-                end
-
-                self.parse(user, text)
-            end
-        end
-
-        # Remove a user's cron tab.
-        def self.remove(user)
-            @tabs[user] ||= @filetype.new(user)
-            @tabs[user].remove
-        end
-
-        # Store the user's cron tab.  Collects the text of the new tab and
-        # sends it to the +@filetype+ module's +write+ function.  Also adds
-        # header, warning users not to modify the file directly.
-        def self.store(user)
-            unless @instances.include?(user) or @objects.find do |n,o|
-                o[:user] == user
-            end
-                Puppet.notice "No cron instances for %s" % user
-                return
-            end
-
-            @tabs[user] ||= @filetype.new(user)
-
-            self.each do |inst|
-                next unless inst[:user] == user
-                unless (@instances[user] and @instances[user].include? inst)
-                    @instances[user] ||= []
-                    @instances[user] << inst
-                end
-            end
-            @tabs[user].write(self.tab(user))
-        end
-
-        # Collect all Cron instances for a given user and convert them
-        # into literal text.
-        def self.tab(user)
-            Puppet.info "Writing cron tab for %s" % user
-            if @instances.include?(user)
-                tab = @instances[user].reject { |obj|
-                    if obj.is_a?(self) and obj.should(:ensure) == :absent
-                        true
-                    else
-                        false
-                    end
-                }.collect { |obj|
-                    if obj.is_a? self
-                        obj.to_record
-                    else
-                        obj.to_s
-                    end
-                }.join("\n") + "\n"
-
-                # Apparently Freebsd will "helpfully" add a new TZ line to every
-                # single cron line, but not in all cases (e.g., it doesn't do it
-                # on my machine.  This is my attempt to fix it so the TZ lines don't
-                # multiply.
-                if tab =~ /^TZ=.+$/
-                    return tab.sub(/\n/, "\n" + self.header)
-                else
-                    return self.header() + tab
-                end
-
-            else
-                Puppet.notice "No cron instances for %s" % user
-            end
-        end
-
-        # Return the tab object itself.  Pretty much just used for testing.
-        def self.tabobj(user)
-            @tabs[user]
-        end
-
-        # Return the last time a given user's cron tab was loaded.  Could
-        # be used for reducing writes, but currently is not.
-        def self.loaded?(user)
-            if @tabs.include?(user)
-                return @loaded[user].loaded
-            else
-                return nil
-            end
-        end
-
-        def create
-            # nothing
-            self.store
-        end
-
-        def destroy
-            # nothing, since the 'Cron.tab' method just doesn't write out
-            # crons whose 'ensure' states are set to 'absent'.
-            self.store
-        end
-
-        def exists?
-            obj = @parameters[:ensure] and obj.is == :present
-        end
-
-        # Override the default Puppet::Type method because we need to call
-        # the +@filetype+ retrieve method.
-        def retrieve
-            unless @parameters.include?(:user)
-                self.fail "You must specify the cron user"
-            end
-
-            self.class.retrieve(self[:user])
-            self.eachproperty { |st|
-                st.retrieve
-            }
-        end
-
-        # Write the entire user's cron tab out.
-        def store
-            self.class.store(self[:user])
-        end
-
-        # Convert the current object a cron-style string.  Adds the cron name
-        # as a comment above the cron job, in the form '# Puppet Name: <name>'.
-        def to_record
-            hash = {}
-
-            # Collect all of the values that we have
-            self.class.fields().each { |param|
-                hash[param] = self.value(param)
-
-                unless hash[param]
-                    devfail "Got no value for %s" % param
-                end
-            }
-
-            str = ""
-
-            str = "# Puppet Name: %s\n" % self.name
-
-            if env = @parameters[:environment] and env.should != :absent
-                envs = env.should
-                unless envs.is_a? Array
-                    envs = [envs]
-                end
-
-                envs.each do |line| str += (line + "\n") end
-            end
-
-            line = nil
-            if special = self.value(:special)
-                line = str + "@%s %s" %
-                    [special, self.value(:command)]
-            else
-                line = str + self.class.fields.collect { |f|
-                    if hash[f] and hash[f] != :absent
-                        hash[f]
-                    else
-                        "*"
-                    end
-                }.join(" ")
-            end
-
-            return line
-        end
-
         def value(name)
             name = symbolize(name)
             ret = nil
diff --git a/lib/puppet/util/fileparsing.rb b/lib/puppet/util/fileparsing.rb
index 8d8d92df9..08f1c79d9 100644
--- a/lib/puppet/util/fileparsing.rb
+++ b/lib/puppet/util/fileparsing.rb
@@ -24,10 +24,75 @@
 # You could then call 'parser.to_line(hash)' on any of those hashes to generate
 # the text line again.
 
+require 'puppet/util/methodhelper'
+
 module Puppet::Util::FileParsing
     include Puppet::Util
     attr_writer :line_separator, :trailing_separator
 
+    class FileRecord
+        include Puppet::Util
+        include Puppet::Util::MethodHelper
+        attr_accessor :absent, :joiner, :rts,
+            :separator, :rollup, :name, :match
+
+        attr_reader :fields, :optional, :type
+
+        INVALID_FIELDS = [:record_type, :target, :on_disk]
+
+        # Customize this so we can do a bit of validation.
+        def fields=(fields)
+            @fields = fields.collect do |field|
+                r = symbolize(field)
+                if INVALID_FIELDS.include?(r)
+                    raise ArgumentError.new("Cannot have fields named %s" % r)
+                end
+                r
+            end
+        end
+
+        def initialize(type, options = {}, &block)
+            @type = symbolize(type)
+            unless [:record, :text].include?(@type)
+                raise ArgumentError, "Invalid record type %s" % @type
+            end
+
+            set_options(options)
+
+            if self.type == :record
+                # Now set defaults.
+                self.absent ||= ""
+                self.separator ||= /\s+/
+                self.joiner ||= " "
+                self.optional ||= []
+                unless defined? @rollup
+                    @rollup = true
+                end
+            end
+
+            if block_given?
+                meta_def(:process, &block)
+            end
+        end
+
+        # Customize this so we can do a bit of validation.
+        def optional=(optional)
+            @optional = optional.collect do |field|
+                symbolize(field)
+            end
+        end
+
+        # Create a hook that modifies the hash resulting from parsing.
+        def post_parse=(block)
+            meta_def(:post_parse, &block)
+        end
+
+        # Create a hook that modifies the hash just prior to generation.
+        def pre_gen=(block)
+            meta_def(:pre_gen, &block)
+        end
+    end
+
     # Clear all existing record definitions.  Only used for testing.
     def clear_records
         @record_types.clear
@@ -35,44 +100,45 @@ module Puppet::Util::FileParsing
     end
 
     def fields(type)
-        type = symbolize(type)
-        if @record_types.include?(type)
-            @record_types[type][:fields].dup
+        if record = record_type(type)
+            record.fields.dup
         else
             nil
         end
     end
 
     # Try to match a specific text line.
-    def handle_text_line(line, hash)
-        if line =~ hash[:match]
-            return {:record_type => hash[:name], :line => line}
+    def handle_text_line(line, record)
+        if line =~ record.match
+            return {:record_type => record.name, :line => line}
         else
             return nil
         end
     end
 
     # Try to match a record.
-    def handle_record_line(line, hash)
-        if method = hash[:method]
-            if ret = send(method, line.dup)
-                ret[:record_type] = hash[:name]
+    def handle_record_line(line, record)
+        if record.respond_to?(:process)
+            if ret = record.send(:process, line.dup)
+                unless ret.is_a?(Hash)
+                    raise Puppet::DevError,
+                        "Process record type %s returned non-hash" % record.name
+                end
+                ret[:record_type] = record.name
                 return ret
             else
                 return nil
             end
-        elsif regex = hash[:match]
+        elsif regex = record.match
             raise "Cannot use matches to handle records yet"
             # In this case, we try to match the whole line and then use the
             # match captures to get our fields.
             if match = regex.match(line)
                 fields = []
-                ignore = hash[:ignore] || []
-                p match.captures
+                ignore = record.ignore || []
                 match.captures.each_with_index do |value, i|
                     fields << value unless ignore.include? i
                 end
-                p fields
                 nil
             else
                 Puppet.info "Did not match %s" % line
@@ -80,21 +146,29 @@ module Puppet::Util::FileParsing
             end
         else
             ret = {}
-            sep = hash[:separator]
+            sep = record.separator
 
             # String "helpfully" replaces ' ' with /\s+/ in splitting, so we
             # have to work around it.
             if sep == " "
                 sep = / /
             end
-            hash[:fields].zip(line.split(sep)) do |param, value|
-                if value and value != ""
+            line_fields = line.split(sep)
+            record.fields.each do |param|
+                value = line_fields.shift
+                if value and value != record.absent
                     ret[param] = value
                 else
                     ret[param] = :absent
                 end
             end
-            ret[:record_type] = hash[:name]
+
+            unless line_fields.empty? or ! record.rollup
+                last_field = record.fields[-1]
+                val = ([ret[last_field]] + line_fields).join(record.joiner)
+                ret[last_field] = val
+            end
+            ret[:record_type] = record.name
             return ret
         end
     end
@@ -127,18 +201,24 @@ module Puppet::Util::FileParsing
         end
 
         @record_order.each do |name|
-            hash = @record_types[name]
-            unless hash
-                raise Puppet::DevError, "Did not get hash for %s: %s" %
+            record = record_type(name)
+            unless record
+                raise Puppet::DevError, "Did not get record type for %s: %s" %
                     [name, @record_types.inspect]
             end
-            method = "handle_%s_line" % hash[:type]
+
+            # These are basically either text or record lines.
+            method = "handle_%s_line" % record.type
             if respond_to?(method)
-                if result = send(method, line, hash)
+                if result = send(method, line, record)
+                    if record.respond_to?(:post_parse)
+                        record.send(:post_parse, result)
+                    end
                     return result
                 end
             else
-                raise Puppet::DevError, "Somehow got invalid line type %s" % hash[:type]
+                raise Puppet::DevError,
+                    "Somehow got invalid line type %s" % record.type
             end
         end
 
@@ -162,42 +242,10 @@ module Puppet::Util::FileParsing
             raise ArgumentError, "Must include a list of fields"
         end
 
-        invalidfields = [:record_type, :target, :on_disk]
-        options[:fields] = options[:fields].collect do |field|
-            r = symbolize(field)
-            if invalidfields.include?(r)
-                raise ArgumentError.new("Cannot have fields named %s" % r)
-            end
-            r
-        end
-
-        options[:absent] ||= ""
-
-        if options[:optional]
-            options[:optional] = options[:optional].collect { |f| symbolize(f) }
-        else
-            options[:optional] = []
-        end
-
-        options[:separator] ||= /\s+/
+        record = FileRecord.new(:record, options, &block)
+        record.name = symbolize(name)
 
-        # Unless they specified a string-based joiner, just use a single
-        # space as the join value.
-        unless options[:separator].is_a?(String) or options[:joiner]
-            options[:joiner] = " "
-        end
-        
-
-        if block_given?
-            method = "handle_record_line_%s" % name
-            if respond_to?(method)
-                raise "Already have a method defined for this record"
-            end
-            meta_def(method, &block)
-            options[:method] = method
-        end
-
-        new_line_type(name, :record, options)
+        new_line_type(record)
     end
 
     # Are there any record types defined?
@@ -206,12 +254,15 @@ module Puppet::Util::FileParsing
     end
 
     # Define a new type of text record.
-    def text_line(name, options)
+    def text_line(name, options, &block)
         unless options.include?(:match)
             raise ArgumentError, "You must provide a :match regex for text lines"
         end
 
-        new_line_type(name, :text, options)
+        record = FileRecord.new(:text, options, &block)
+        record.name = symbolize(name)
+
+        new_line_type(record)
     end
 
     # Generate a file from a bunch of hash records.
@@ -227,20 +278,25 @@ module Puppet::Util::FileParsing
 
     # Convert our parsed record into a text record.
     def to_line(details)
-        unless type = @record_types[details[:record_type]]
+        unless record = record_type(details[:record_type])
             raise ArgumentError, "Invalid record type %s" % details[:record_type]
         end
 
-        case type[:type]
+        if record.respond_to?(:pre_gen)
+            details = details.dup
+            record.send(:pre_gen, details)
+        end
+
+        case record.type
         when :text: return details[:line]
         else
-            joinchar = type[:joiner] || type[:separator]
+            joinchar = record.joiner
 
-            line = type[:fields].collect { |field|
+            line = record.fields.collect { |field|
                 # If the field is marked absent, use the appropriate replacement
                 if details[field] == :absent or details[field].nil?
-                    if type[:optional].include?(field)
-                        type[:absent]
+                    if record.optional.include?(field)
+                        record.absent
                     else
                         raise ArgumentError, "Field %s is required" % field
                     end
@@ -249,7 +305,7 @@ module Puppet::Util::FileParsing
                 end
             }.reject { |c| c.nil?}.join(joinchar)
 
-            if regex = type[:rts]
+            if regex = record.rts
                 # If they say true, then use whitespace; else, use their regex.
                 if regex == true
                     regex = /\s+$/
@@ -272,7 +328,7 @@ module Puppet::Util::FileParsing
 
     def valid_attr?(type, attr)
         type = symbolize(type)
-        if @record_types[type] and @record_types[type][:fields].include?(symbolize(attr))
+        if record = record_type(type) and record.fields.include?(symbolize(attr))
             return true
         else
             if symbolize(attr) == :ensure
@@ -285,22 +341,23 @@ module Puppet::Util::FileParsing
 
     private
     # Define a new type of record.
-    def new_line_type(name, type, options)
+    def new_line_type(record)
         @record_types ||= {}
         @record_order ||= []
 
-        name = symbolize(name)
-
-        if @record_types.include?(name)
-            raise ArgumentError, "Line type %s is already defined" % name
+        if @record_types.include?(record.name)
+            raise ArgumentError, "Line type %s is already defined" % record.name
         end
 
-        options[:name] = name
-        options[:type] = type
-        @record_types[name] = options
-        @record_order << name
+        @record_types[record.name] = record
+        @record_order << record.name
+
+        return record
+    end
 
-        return options
+    # Retrive the record object.
+    def record_type(type)
+        @record_types[symbolize(type)]
     end
 end
 
diff --git a/lib/puppet/util/methodhelper.rb b/lib/puppet/util/methodhelper.rb
index 7de723c3b..63158ab67 100644
--- a/lib/puppet/util/methodhelper.rb
+++ b/lib/puppet/util/methodhelper.rb
@@ -15,7 +15,7 @@ module Puppet::Util::MethodHelper
             begin
                 self.send(method, value)
             rescue NoMethodError
-                self.fail "Invalid parameter %s to object class %s" %
+                raise ArgumentError, "Invalid parameter %s to object class %s" %
                         [param,self.class.to_s]
             end
         end