Andrew Ferguson wrote:
On Jun 26, 2008, at 10:48 PM, Josh Nisly wrote:
Fred Gansevles found some problems with the previous patch. Attached
is an updated one.
Josh Nisly wrote:
Attached is a patch that fixes any problems I know about with the
previous one, and includes several fixes by Fred Gansevles. It
serializes the ACL record to string in the RPath member, which
allows new versions to work with existing servers. I've also
implemented the correct checks for whether the remote connection
supports ACLs, so backing up a Windows client to a linux server
should work fine.
Comments are welcome.
Josh and Fred,
This patch is looking very good, thanks. I have a few comments:
Thanks for the feedback. I've attached a patch that address your
comments below.
Index: rdiff_backup/connection.py
===================================================================
RCS file:
/sources/rdiff-backup/rdiff-backup/rdiff_backup/connection.py,v
retrieving revision 1.29
diff -u -r1.29 connection.py
--- rdiff_backup/connection.py 9 Jul 2007 03:53:40 -0000 1.29
+++ rdiff_backup/connection.py 23 Jun 2008 06:35:52 -0000
@@ -539,6 +540,9 @@
TempFile, SetConnections, librsync, log, regress, fs_abilities, \
eas_acls, user_group, compare
+try: import win_acls
+except: pass
+
Is it ok if this is "except ImportError" instead? (Seems obvious to
me, just wanted to double-check)
Sure, changed in attached patch.
Index: rdiff_backup/rpath.py
===================================================================
RCS file: /sources/rdiff-backup/rdiff-backup/rdiff_backup/rpath.py,v
retrieving revision 1.120
diff -u -r1.120 rpath.py
--- rdiff_backup/rpath.py 10 Jun 2008 13:14:52 -0000 1.120
+++ rdiff_backup/rpath.py 26 Jun 2008 06:44:09 -0000
@@ -257,6 +258,8 @@
if error.errno != errno.EEXIST: raise
# On Windows, files can't be renamed on top of an
existing file
+ try: rp_source.conn.os.chmod(rp_dest.path,
stat.S_IWRITE)
+ except: pass
rp_source.conn.os.unlink(rp_dest.path)
rp_source.conn.os.rename(rp_source.path, rp_dest.path)
Why is this change here? Does Windows let you do the rename if you
have write permissions? Doing this chmod here is not something we
want, especially not blindly catching all exceptions without resetting
the permissions afterwards.
Secondly, I actually shouldn't have committed this Windows rename fix,
anyway. On POSIX, rename() is supposed to be an atomic operation.
since this exception branch we added for Windows doesn't check for
os.name == 'nt', it breaks the atomic nature of rename() on POSIX.
If you look at the rpath.move() function, you'll see that it catches
the os.error thrown by a failed rename() and does the equivalent of
unlink() / rename() by doing copy(rpin, rpout), rpin.delete().
So, I see two possible fixes for this problem:
1) Make the windows-specific code actually windows-specific
2) Go through the 10 calls to rpath.rename() and see if they depend on
the atomic assumption. If not, they can safely be made into
rpath.move() calls, and the Windows-specific code can be dropped.
Yes, I thought about the atomic problem a while back. IIUC, the atomic
nature is required when backing up, to ensure that if a backup fails,
the repo can be regressed. However, this is only a problem if the server
is Windows, which is a setup I don't really think will happen very
often. Nevertheless, we should make it work correctly.
There is a Win32 API function MoveFileEx(), that can be told to rename
over existing files. I think that the right solution is to have
fs_abilities.py check whether a file can be renamed over an existing
file, and if it can't, check if the MoveFileEx is available and use it.
(If neither work, rdiff-backup could error out.)
--- rdiff_backup/win_acls.py.orig Thu Jun 26 10:32:06 2008
+++ rdiff_backup/win_acls.py Thu Jun 26 10:04:27 2008
@@ -0,0 +1,201 @@
+# Copyright 2008 Fred Gansevles <[EMAIL PROTECTED]>
+#
+# This file is part of rdiff-backup.
+#
+# rdiff-backup is free software; you can redistribute it and/or modify
+# under the terms of the GNU General Public License as published by the
+# Free Software Foundation; either version 2 of the License, or (at
your
+# option) any later version.
+#
+# rdiff-backup is distributed in the hope that it will be useful, but
+# WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+# General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with rdiff-backup; if not, write to the Free Software
+# Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307
+# USA
+
+__version__ = (0, 1, 1)
Is this necessary? We don't do that anywhere else in rdiff-backup.
Nope. Removed in the attached patch.
+import C, metadata, re, rorpiter, rpath
+
+try:
+ from win32security import *
+except:
+ GROUP_SECURITY_INFORMATION = 0
+ OWNER_SECURITY_INFORMATION = 0
+ DACL_SECURITY_INFORMATION = 0
+
Are these globals which are imported by win32security? If not,
rdiff-backup doesn't have globals in all caps.
Yes, these come from win32security.
I'll defer to your expertise on the rest of win_acls.py. Let me know
about these few things and I'll commit the whole patch to CVS.
Andrew
Thanks for your work on this stuff!
JoshN
_______________________________________________
rdiff-backup-users mailing list at [email protected]
http://lists.nongnu.org/mailman/listinfo/rdiff-backup-users
Wiki URL: http://rdiff-backup.solutionsfirst.com.au/index.php/RdiffBackupWiki
