-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 Ok, I started doing the suggested code changes, but didn't get very far.
When munge returns something other than an integer, something somewhere
blows up and I can't tell what or why.
sync and insync? don't appear to be activated at all.
The trace is attached from running the following:
puppet --test --trace test.pp
test.pp contains:
file { "/tmp/foo":
ensure => 'file',
mode => 'o=u,o-w',
content => 'foo'
}
The mode string is being returned by munge.
Any clues would be appreciated.
Also, I'm not going to try to refactor the munge method. It seems fine
to me (but that's probably why I'm not a full time Ruby programmer) so
I'm not really sure what you're looking for.
Trevor
On 01/13/2010 05:17 PM, Luke Kanies wrote:
> (Finally reviewing code.)
>
> There's a bit of a strategical flaw in this code - symbolic modes are
> usually incomplete specifications, like u+w,g-w, but this code makes
> them complete specs. E.g., if I say 'mode => 'u+w,o-w', this would
> translate that to '200', but really we'd just want to make sure the user
> had write access and other didn't.
>
> So this is more about changing the 'insync?' and 'sync' methods than the
> munging. We need 'insync?' to be able to compare the specified bits,
> probably using something like a mask, and we need 'sync' to know how to
> change just the bits that need to be changed.
>
> Also, a couple of comments below.
>
> On Jan 4, 2010, at 6:42 PM, Trevor Vaughan - Onyx Point wrote:
>
>> From: Trevor Vaughan <[email protected]>
>>
>>
>> Signed-off-by: Trevor Vaughan - Onyx Point <[email protected]>
>> ---
>> lib/puppet/type/file/mode.rb | 115
>> +++++++++++++++++++++++++++++++++--------
>> 1 files changed, 92 insertions(+), 23 deletions(-)
>>
>> diff --git a/lib/puppet/type/file/mode.rb b/lib/puppet/type/file/mode.rb
>> index 83034cb..dd582cf 100755
>> --- a/lib/puppet/type/file/mode.rb
>> +++ b/lib/puppet/type/file/mode.rb
>> @@ -4,9 +4,8 @@
>> module Puppet
>> Puppet::Type.type(:file).newproperty(:mode) do
>> require 'etc'
>> - desc "Mode the file should be. Currently relatively limited:
>> - you must specify the exact mode the file should be.
>> -
>> + desc "Mode the file should be. You may specify either the
>> precise octal mode or the POSIX symbolic mode per GNU coreutils chmod.
>> +
>> Note that when you set the mode of a directory, Puppet always
>> sets the search/traverse (1) bit anywhere the read (4) bit
>> is set.
>> This is almost always what you want: read allows you to
>> list the
>> @@ -22,9 +21,74 @@ module Puppet
>>
>> In this case all of the files underneath ``/some/dir``
>> will have
>> mode 644, and all of the directories will have mode 755."
>> -
>> @event = :file_changed
>>
>> + # This is a helper that takes a symbolic string and a file
>> path and
>> + # returns the adjusted decimal representation octal file mode
>> (0700,
>> + # etc...)
>> + def sym2oct(str,path)
>> + if str.to_s =~ /^\d+$/
>> + value = str
>> + else
>> + curmode = "00000"
>> + if File.exists?(path) then
>> + curmode = "%o" % File.stat(path).mode
>> + curmode = curmode[-5 .. -1]
>> + end
>> + value = Integer(curmode)
>> +
>> + base = {
>> + "u" => 6,
>> + "g" => 3,
>> + "o" => 0
>> + }
>> +
>> + left = {
>> + "u" => 05700,
>> + "g" => 03070,
>> + "o" => 01007,
>> + "a" => 07777
>> + }
>> +
>> + right = {
>> + "r" => 00444,
>> + "w" => 00222,
>> + "x" => 00111,
>> + "s" => 06000,
>> + "t" => 01000,
>> + "u" => 00700,
>> + "g" => 00070,
>> + "o" => 00007
>> + }
>> +
>> + reg = /^(([ugoa]+)([+-=])([rwxst]+|[ugo]),?)+$/
>> +
>> + str.split(",").each do |cmd|
>> + match = cmd.match(reg) or return curmode
>> + # The following vars are directly dependent on the
>> + # structure of the regex (reg) above
>> + who = match[2]
>> + what = match[3]
>> + how = match[4].split(//).uniq.to_s
>> + if how =~ /^[ugo]$/ then
>> + who.split(//).uniq.each do |lhv|
>> + right[how] = ( ((value << (base[lhv] -
>> base[how])) & right[lhv]) | ( value & ~right[lhv] ) ) & 0777
>> + end
>> + end
>> + who = who.split(//).inject(num=0) {|num,b| num |=
>> left[b]; num }
>> + how = how.split(//).inject(num=0) {|num,b| num |=
>> right[b]; num }
>> + mask = who & how
>> + case what
>> + when "+": value = value | mask
>> + when "-": value = value & ~mask
>> + when "=": value = ( mask & who ) | ( value &
>> ~(who & 0777) )
>> + end
>> + end
>> + end
>> + Integer("0%o" % value)
>> + end
>> +
>> +
>
> This whole patch is missing tests, but this method especially needs
> tests. It'd be good to have a bunch of example inputs and expected
> outputs, with verification that it does what we expect.
>
> It'd also be good, stylistically, to see the method broken up a bit,
> with the static hashes turned into constants. It might even be worth
> moving the symbol munging into a module, but that might be overkill.
>
>> # Our modes are octal, so make sure they print correctly. Other
>> # valid values are symbols, basically
>> def is_to_s(currentvalue)
>> @@ -52,29 +116,35 @@ module Puppet
>> end
>>
>> munge do |should|
>> - # this is pretty hackish, but i need to make sure the
>> number is in
>> - # octal, yet the number can only be specified as a string
>> right now
>> + # This handles both numbers and symbolic modes matching
>> the regex
>> + # /^(([ugoa]+)([+-=])([rwx]+),?)$/
>> + #
>> + # Note: This now returns a string and the accepting
>> function must
>> + # know how to handle it!
>> + lregex = /^(([ugoa]+)([+-=])([rwxst]+|[ugo]),?)+$/
>> +
>> value = should
>> +
>> if value.is_a?(String)
>> - unless value =~ /^\d+$/
>> - raise Puppet::Error, "File modes can only be
>> numbers, not %s" %
>> + if value =~ /^\d+$/ then
>> + unless value =~ /^0/
>> + value = "0#{value}"
>> + end
>> +
>> + old = value
>> + begin
>> + value = Integer(value)
>> + rescue ArgumentError => detail
>> + raise Puppet::DevError, "Could not convert %s
>> to integer" %
>> + old.inspect
>> + end
>> + elsif value.match(lregex).nil? then
>> + raise Puppet::DevError, "Symbolic mode %s does
>> not match #{lregex}" %
>> value.inspect
>> end
>> - # Make sure our number looks like octal.
>> - unless value =~ /^0/
>> - value = "0" + value
>> - end
>> - old = value
>> - begin
>> - value = Integer(value)
>> - rescue ArgumentError => detail
>> - raise Puppet::DevError, "Could not convert %s to
>> integer" %
>> - old.inspect
>> - end
>> end
>> -
>> - return value
>> - end
>> + return sym2oct(value,@resource[:path])
>> + end
>
> This call was already in need of a refactor, and this additional code
> makes it cry out for one. It'd be good to see this code broken up into
> multiple small methods, with this block just calling out to them as
> necessary, rather than having the whole block inline.
>
>> # If we're a directory, we need to be executable for all cases
>> # that are readable. This should probably be selectable, but eh.
>> @@ -121,7 +191,6 @@ module Puppet
>>
>> def sync
>> mode = self.should
>> -
>> begin
>> File.chmod(mode, @resource[:path])
>> rescue => detail
>> --
>> 1.6.2.5
>>
>> --
>>
>> You received this message because you are subscribed to the Google
>> Groups "Puppet Developers" group.
>> To post to this group, send email to [email protected].
>> To unsubscribe from this group, send email to
>> [email protected].
>> For more options, visit this group at
>> http://groups.google.com/group/puppet-dev?hl=en.
>>
>>
>
>
- --
Trevor Vaughan
Vice President, Onyx Point, Inc.
email: [email protected]
phone: 410-541-ONYX (6699)
- -- This account not approved for unencrypted sensitive information --
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (GNU/Linux)
iEYEARECAAYFAktP0x0ACgkQyWMIJmxwHpRAjQCfScVV73yXP+GdagqstO6LrR/J
UzwAn3csEqE8Uaocz2SWRyosqYflpmX/
=cg8h
-----END PGP SIGNATURE-----
[0;37mdebug: Creating default schedules[0m [0;37mdebug: Finishing transaction -610974828 with 0 changes[0m [0;37mdebug: //Exec[/bin/ls -l /tmp/foo]/subscribe: subscribes to File[/tmp/foo][0m [0;37mdebug: Failed to load library 'ldap' for feature 'ldap'[0m [0;32minfo: Applying configuration version '1263522229'[0m [0;37mdebug: //File[/tmp/foo]: Changing ensure[0m [0;37mdebug: //File[/tmp/foo]: 1 change(s)[0m /home/bob/Applications/Puppet/usr/lib/ruby/site_ruby/1.8/puppet/type/file.rb:732:in `initialize' /home/bob/Applications/Puppet/usr/lib/ruby/site_ruby/1.8/puppet/type/file.rb:732:in `open' /home/bob/Applications/Puppet/usr/lib/ruby/site_ruby/1.8/puppet/type/file.rb:732:in `write' /home/bob/Applications/Puppet/usr/lib/ruby/site_ruby/1.8/puppet/util.rb:145:in `withumask' /home/bob/Applications/Puppet/usr/lib/ruby/site_ruby/1.8/puppet/type/file.rb:731:in `write' /home/bob/Applications/Puppet/usr/lib/ruby/site_ruby/1.8/puppet/type/file/content.rb:141:in `sync' /home/bob/Applications/Puppet/usr/lib/ruby/site_ruby/1.8/puppet/type/file/ensure.rb:46:in `set_file' /home/bob/Applications/Puppet/usr/lib/ruby/site_ruby/1.8/puppet/property.rb:109:in `send' /home/bob/Applications/Puppet/usr/lib/ruby/site_ruby/1.8/puppet/property.rb:109:in `call_valuemethod' /home/bob/Applications/Puppet/usr/lib/ruby/site_ruby/1.8/puppet/property.rb:298:in `set' /home/bob/Applications/Puppet/usr/lib/ruby/site_ruby/1.8/puppet/property.rb:368:in `sync' /home/bob/Applications/Puppet/usr/lib/ruby/site_ruby/1.8/puppet/type/file/ensure.rb:179:in `sync' /home/bob/Applications/Puppet/usr/lib/ruby/site_ruby/1.8/puppet/transaction/change.rb:54:in `go' /home/bob/Applications/Puppet/usr/lib/ruby/site_ruby/1.8/puppet/transaction/change.rb:72:in `forward' /home/bob/Applications/Puppet/usr/lib/ruby/site_ruby/1.8/puppet/transaction.rb:120:in `apply_changes' /home/bob/Applications/Puppet/usr/lib/ruby/site_ruby/1.8/puppet/transaction.rb:113:in `collect' /home/bob/Applications/Puppet/usr/lib/ruby/site_ruby/1.8/puppet/transaction.rb:113:in `apply_changes' /home/bob/Applications/Puppet/usr/lib/ruby/site_ruby/1.8/puppet/transaction.rb:85:in `apply' /home/bob/Applications/Puppet/usr/lib/ruby/site_ruby/1.8/puppet/transaction.rb:251:in `eval_children_and_apply_resource' /home/bob/Applications/Puppet/usr/lib/ruby/site_ruby/1.8/puppet/util.rb:418:in `thinmark' /usr/lib/ruby/gems/1.8/gems/activesupport-2.3.5/lib/active_support/core_ext/benchmark.rb:10:in `realtime' /home/bob/Applications/Puppet/usr/lib/ruby/site_ruby/1.8/puppet/util.rb:417:in `thinmark' /home/bob/Applications/Puppet/usr/lib/ruby/site_ruby/1.8/puppet/transaction.rb:250:in `eval_children_and_apply_resource' /home/bob/Applications/Puppet/usr/lib/ruby/site_ruby/1.8/puppet/transaction.rb:207:in `eval_resource' /home/bob/Applications/Puppet/usr/lib/ruby/site_ruby/1.8/puppet/transaction.rb:296:in `evaluate' /home/bob/Applications/Puppet/usr/lib/ruby/site_ruby/1.8/puppet/util.rb:418:in `thinmark' /usr/lib/ruby/gems/1.8/gems/activesupport-2.3.5/lib/active_support/core_ext/benchmark.rb:10:in `realtime' /home/bob/Applications/Puppet/usr/lib/ruby/site_ruby/1.8/puppet/util.rb:417:in `thinmark' /home/bob/Applications/Puppet/usr/lib/ruby/site_ruby/1.8/puppet/transaction.rb:295:in `evaluate' /home/bob/Applications/Puppet/usr/lib/ruby/site_ruby/1.8/puppet/transaction.rb:289:in `collect' /home/bob/Applications/Puppet/usr/lib/ruby/site_ruby/1.8/puppet/transaction.rb:289:in `evaluate' /home/bob/Applications/Puppet/usr/lib/ruby/site_ruby/1.8/puppet/resource/catalog.rb:142:in `apply' /home/bob/Applications/Puppet/usr/lib/ruby/site_ruby/1.8/puppet/application/puppet.rb:128:in `main' /home/bob/Applications/Puppet/usr/lib/ruby/site_ruby/1.8/puppet/application.rb:226:in `send' /home/bob/Applications/Puppet/usr/lib/ruby/site_ruby/1.8/puppet/application.rb:226:in `run_command' /home/bob/Applications/Puppet/usr/lib/ruby/site_ruby/1.8/puppet/application.rb:217:in `run' /home/bob/Applications/Puppet/usr/lib/ruby/site_ruby/1.8/puppet/application.rb:306:in `exit_on_fail' /home/bob/Applications/Puppet/usr/lib/ruby/site_ruby/1.8/puppet/application.rb:217:in `run' /home/bob/bin/puppet:71 /home/bob/Applications/Puppet/usr/lib/ruby/site_ruby/1.8/puppet/type/file.rb:732:in `initialize' /home/bob/Applications/Puppet/usr/lib/ruby/site_ruby/1.8/puppet/type/file.rb:732:in `open' /home/bob/Applications/Puppet/usr/lib/ruby/site_ruby/1.8/puppet/type/file.rb:732:in `write' /home/bob/Applications/Puppet/usr/lib/ruby/site_ruby/1.8/puppet/util.rb:145:in `withumask' /home/bob/Applications/Puppet/usr/lib/ruby/site_ruby/1.8/puppet/type/file.rb:731:in `write' /home/bob/Applications/Puppet/usr/lib/ruby/site_ruby/1.8/puppet/type/file/content.rb:141:in `sync' /home/bob/Applications/Puppet/usr/lib/ruby/site_ruby/1.8/puppet/type/file/ensure.rb:46:in `set_file' /home/bob/Applications/Puppet/usr/lib/ruby/site_ruby/1.8/puppet/property.rb:109:in `send' /home/bob/Applications/Puppet/usr/lib/ruby/site_ruby/1.8/puppet/property.rb:109:in `call_valuemethod' /home/bob/Applications/Puppet/usr/lib/ruby/site_ruby/1.8/puppet/property.rb:298:in `set' /home/bob/Applications/Puppet/usr/lib/ruby/site_ruby/1.8/puppet/property.rb:368:in `sync' /home/bob/Applications/Puppet/usr/lib/ruby/site_ruby/1.8/puppet/type/file/ensure.rb:179:in `sync' /home/bob/Applications/Puppet/usr/lib/ruby/site_ruby/1.8/puppet/transaction/change.rb:54:in `go' /home/bob/Applications/Puppet/usr/lib/ruby/site_ruby/1.8/puppet/transaction/change.rb:72:in `forward' /home/bob/Applications/Puppet/usr/lib/ruby/site_ruby/1.8/puppet/transaction.rb:120:in `apply_changes' /home/bob/Applications/Puppet/usr/lib/ruby/site_ruby/1.8/puppet/transaction.rb:113:in `collect' /home/bob/Applications/Puppet/usr/lib/ruby/site_ruby/1.8/puppet/transaction.rb:113:in `apply_changes' /home/bob/Applications/Puppet/usr/lib/ruby/site_ruby/1.8/puppet/transaction.rb:85:in `apply' /home/bob/Applications/Puppet/usr/lib/ruby/site_ruby/1.8/puppet/transaction.rb:251:in `eval_children_and_apply_resource' /home/bob/Applications/Puppet/usr/lib/ruby/site_ruby/1.8/puppet/util.rb:418:in `thinmark' /usr/lib/ruby/gems/1.8/gems/activesupport-2.3.5/lib/active_support/core_ext/benchmark.rb:10:in `realtime' /home/bob/Applications/Puppet/usr/lib/ruby/site_ruby/1.8/puppet/util.rb:417:in `thinmark' /home/bob/Applications/Puppet/usr/lib/ruby/site_ruby/1.8/puppet/transaction.rb:250:in `eval_children_and_apply_resource' /home/bob/Applications/Puppet/usr/lib/ruby/site_ruby/1.8/puppet/transaction.rb:207:in `eval_resource' /home/bob/Applications/Puppet/usr/lib/ruby/site_ruby/1.8/puppet/transaction.rb:296:in `evaluate' /home/bob/Applications/Puppet/usr/lib/ruby/site_ruby/1.8/puppet/util.rb:418:in `thinmark' /usr/lib/ruby/gems/1.8/gems/activesupport-2.3.5/lib/active_support/core_ext/benchmark.rb:10:in `realtime' /home/bob/Applications/Puppet/usr/lib/ruby/site_ruby/1.8/puppet/util.rb:417:in `thinmark' /home/bob/Applications/Puppet/usr/lib/ruby/site_ruby/1.8/puppet/transaction.rb:295:in `evaluate' /home/bob/Applications/Puppet/usr/lib/ruby/site_ruby/1.8/puppet/transaction.rb:289:in `collect' /home/bob/Applications/Puppet/usr/lib/ruby/site_ruby/1.8/puppet/transaction.rb:289:in `evaluate' /home/bob/Applications/Puppet/usr/lib/ruby/site_ruby/1.8/puppet/resource/catalog.rb:142:in `apply' /home/bob/Applications/Puppet/usr/lib/ruby/site_ruby/1.8/puppet/application/puppet.rb:128:in `main' /home/bob/Applications/Puppet/usr/lib/ruby/site_ruby/1.8/puppet/application.rb:226:in `send' /home/bob/Applications/Puppet/usr/lib/ruby/site_ruby/1.8/puppet/application.rb:226:in `run_command' /home/bob/Applications/Puppet/usr/lib/ruby/site_ruby/1.8/puppet/application.rb:217:in `run' /home/bob/Applications/Puppet/usr/lib/ruby/site_ruby/1.8/puppet/application.rb:306:in `exit_on_fail' /home/bob/Applications/Puppet/usr/lib/ruby/site_ruby/1.8/puppet/application.rb:217:in `run' /home/bob/bin/puppet:71 [1;35merr: //File[/tmp/foo]/ensure: change from absent to file failed: Could not set file on ensure: can't convert String into Integer at /srv/home/bob/Puppet/puppet/../test.pp:8[0m [0;36mnotice: //Exec[/bin/ls -l /tmp/foo]: Dependency file[/tmp/foo] has 1 failures[0m [0;33mwarning: //Exec[/bin/ls -l /tmp/foo]: Skipping because of failed dependencies[0m [0;37mdebug: Finishing transaction -610980958 with 1 changes[0m
puptrace.txt.sig
Description: PGP signature
-- You received this message because you are subscribed to the Google Groups "Puppet Developers" group. To post to this group, send email to [email protected]. To unsubscribe from this group, send email to [email protected]. For more options, visit this group at http://groups.google.com/group/puppet-dev?hl=en.
