PATCH (Re: Changes to copyfile functionality)

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