(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
.
--
Computers are not intelligent. They only think they are.
---------------------------------------------------------------------
Luke Kanies -|- http://reductivelabs.com -|- +1(615)594-8199
--
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.
