Re: PATCH (Re: Changes to copyfile functionality)

Attached is an updated version of this patch which makes two more changes to the CopyFile minion. One major change is that it now tries to use the pyxattr library to copy extended attributes (including SELinux contexts) from the target file to the tempfile. Pyxattr is not a core library but it is packaged for RHEL5 and 6. If this library is not available then it falls back to calling chcon. The other major change is that it explicitly looks for a SELinux config file. If one is present then it will consider a failure to copy the SELinux contexts from the target file to the tempfile to be an error and will exit. Previously it just kept going when this failed. The remaining changes are organizational, e.g. moving the "from subprocess import..." to the top. -- Marcus On Monday, September 03, 2012 04:29:28 PM Marcus Lauer wrote: > Since part #1 of my three-part plan is proving > troublesome I moved> > ahead on #2. > > Although part #3 of my original plan borrows some code > from #1 it > > does not require #1. If this patch is accepted then I will move ahead > on > #3. > > This patch changes copyfile in two ways: > 1. Copyfile now opens a tempfile on the minion and appends data to this > tempfile, then moves the tempfile over the target file after all of the > data has been appended. > > The advantage of this is that updating the target file > is atomic, or> > as close to it as possible in python. This minimizes the chance of some > program accessing an empty or half-complete file. If the file in > question were a server config file... surely you can imagine how this > could be a problem. > > The tempfiles are created in the same directory as the > target file> > (for security) and have the same name as the target file but with the > extension ".functmp". Permissions are set on this file during the > "open" > stage, while it is still zero-size. > > 2. Copyfile has a new option called "retainperms" ("--retainperms" at > the > command line). It is "False" by default. > > Currently func tries to give the new file on the minion > the same > > attributes (permissions and uid/gid) as the file on the overlord. The > SELinux context of the new file is not set (and does not change). This > overrides this behavior to make the permissions, flags, uid, gid and > SELinux contexts of the new file identical to the existing file (if any) > on the minion. > > This is intended to deal with the situation in which the > file on the> > overlord has different permissions than the files on the minions and the > user does not want to overwrite them. Perhaps the overlord computer > does > not have the same users and groups as the minions? I figured this might > be useful for someone. > > On a related note, does anyone think it would be > worthwhile to have> > copyfile copy the SELinux contexts of the source file to the target? > How > about making command-line options to allow the user to specify the > desired mode, uid, gid, contexts, etc.? > > -- > Marcus > > On Saturday, August 25, 2012 04:55:09 PM Marcus Lauer wrote: > > Attached are my original patch and a git-patch which > > contains the > > > > same code. These implement changes I called #1 in my previous > > e-mail. > > > > #2 doesn't seem too hard. CopyFile.open will have > > to > > open a > > > > tempfile rather than the target file. It already returns the > > filepath > > on > > success. Append already uses the return value of CopyFile.open as > > the > > file to append to. After appending has finished there needs to be a > > CopyFile.move function called by the overlord which takes both paths > > and moves the tempfile over the source file. Making sure the > > ownership/permissions/SELinux attributes of the target file do not > > change > > could be is important so had better be careful about that. > > > > Once this is in place #3 is almost trivial. The > > overlord needs to> > > > > calculate the MD5 of the source file. The code for doing this is > > also > > needed by #1 and is included in my patches. That MD5 should be > > passed > > to > > the CopyFile.move function. It should calculate the MD5 of the > > tempfile and if they don't match just delete the tempfile and > > return an error instead of moving it. I figure that this MD5 check > > should be optional so I will also create a flag to enable it. > > > > Can anyone see any issues with this general design? > > If > > not I'll get> > > > > working on it when I have the time. > > > > -- > > Marcus > > > > On Friday, August 24, 2012 07:32:14 PM you wrote: > > > Copyfile could most certainly use some TLC, so let me know if I > > > can > > > help, > > > that would certainly be of help if you'd like to work on that. > > > The > > > list has indeed changed to lists.fedorahosted.com, so definitely > > > use that one. > > > > > > Would you mind shooting me your original patch as well? Thanks > > > very > > > much!> > > > On Fri, Aug 24, 2012 at 7:22 PM, Marcus Lauer > > <marcuslauer(a)nyc.rr.com&gt;wrote: > > > > It seems to me that there are at least > > > > three > > > > improvements which > > > > > > > > should be made to copyfile: > > > > > > > > 1. If the file on the overlord and minion are the same, that > > > > file > > > > should not > > > > be copied. This saves bandwidth/processing time. > > > > > > > > 2. Overwriting the file on the minion should be atomic, e.g. > > > > the > > > > file on the > > > > overlord should be copied to a tempfile on the minion then > > > > moved > > > > to > > > > overwrite > > > > the target file as a final step. Right now if funcd dies or > > > > something > > > > goes wrong mid-copy you end up with half a file on the > > > > minion. > > > > > > > > 3. There should be an option (off by default) such that if > > > > the > > > > file > > > > which > > > > has > > > > been copied over to the minion is not identical to the file > > > > on > > > > the > > > > overlord it > > > > is re-copied, or at least does not overwrite the file on the > > > > minion. > > > > This won't work for some files (busy logfiles) but for most > > > > files it > > > > seems like an > > > > extra bit of reliability which most people would want in > > > > most > > > > situations. > > > > > > > > I submitted a patch (though not a > > > > git-patch... > > > > my > > > > bad!) > > > > back in > > > > > > > > January 2011 which implements #1, but imperfectly. The > > > > overlord > > > > still > > > > has to > > > > send a (small) packet to the minion for each 60KB chunk of > > > > target > > > > file. > > > > > > > > Would anyone be bothered terribly if I worked > > > > on > > > > #2 > > > > and > > > > #3? While > > > > > > > > I am > > > > at it I will try to find a way to fix up my patch for #1 as > > > > well. > > > > There is some overlap between these patches. Both will > > > > involve > > > > calculating an MD5 hash > > > > of the file on the overlord, for example. > > > > > > > > P.S. Is the new mailing list on > > > > fedorahosted.org > > > > live > > > > yet? I am > > > > > > > > sending this message to both just in case. > > > > > > > > -- > > > > Marcus > > > > _______________________________________________ > > > > func mailing list > > > > func(a)lists.fedorahosted.org > > > > https://lists.fedorahosted.org/mailman/listinfo/func > > > > > > -- > > > Steve Salevan > > > steve(a)tumblr.com