Re: [rdiff-backup-users] [PATCH] Backing up Windows ACLs

[Josh Nisly]

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 <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.
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