[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [rdiff-backup-users] [PATCH] Backing up Windows ACLs
From: |
Andrew Ferguson |
Subject: |
Re: [rdiff-backup-users] [PATCH] Backing up Windows ACLs |
Date: |
Fri, 27 Jun 2008 14:38:35 -0400 |
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:
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)
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.
--- 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 <address@hidden>
+#
+# 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.
+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.
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