On Thu, Dec 10, 2009 at 12:37 AM, James Antill <ja...@fedoraproject.org> wrote: > On Wed, 2009-12-09 at 17:14 -0500, Josef Bacik wrote: >> Hello, >> >> This is my first yum plugin and maybe my second time writing python, >> so any comments would be greatly appreciated. This plugin simply >> takes any directory that a package is going to mess with that belongs >> to a btrfs filesystem while updating/removing the package and takes a >> snapshot of that btrfs filesystem. The idea here is that if something >> catastrophic goes wrong, we can always boot into an older snapshot and >> try and pick up the pieces. I've tested this plugin on my box with a >> simple package removal and the resulting snapshot is clean, the rpmdb >> is good and everything is kosher. Thanks, > > Mostly cosmetic stuff: > > > 1. Don't do init stuff in init_hook :). Because your plugin only ever > needs to do anything when we run a transaction, delay everything until > at least pretrans_hook time. > > 2. In python: > > dirlist = dirlist + [mntpnt] > > ...is done as: > > dirlist.append(mntpnt) > > 3. Why do you only care about update/erase packages? Install packages > will run scriplets etc. ... and TS_OBSOLETING is a synonym for TS_UPDATE > (and most everything else :) dito. TS_OBSOLETED vs. TS_ERASE. > Also in _probably rare_ cases TS_UPDATED packages could have files in > very different places than their TS_UPDATING counterparts. >
The idea was that installing a new application isn't going to cause anything else to regress, but I forgot about scriptlets, I will change this. > 4. Accessing ts.po.dirlist for a "remote" package means the user needs > to download the filelist MD. This isn't easy to fix :(. > I guess the same thing goes for po.filelist and such right? > 5. You don't take bind() mounts into account > Somebody always has to bring up bind() mounts ;). I will see what can be done about this. > 6. Opt: Presort dirlist by len(), then you can break on the first match. > > 7. snaplist.index(); del == snaplist.remove() (or even better, use a > set() and do snaplist.del()). > > 8. I can see why you're doing the scan, and only snapshoting "what will > change", but I think this can only lead to madness: > > i. Lots of other stuff goes on, Eg. yum history, yumdb, rpmdb. > ii. Even if you account for all of that ... scriplets can (and > do) do anything, anywhere. > > ...you want this to be a non-default option, I think, and just snapshot > all of snaplist by default. But that's IMHO. > So the idea here is if the user setup a different subvolume for /home that they don't necessarily want /home snapshotted every time they do an update. Packages shouldn't be touching users home directory right? I guess it wouldn't be terrible to do everything, and it would certainly make things simpler, I'll have to think about it. > 9. Does it output anything on the error path? > Hmm no, I wanted to be as not verbose as possible so users wouldn't freak out if they saw errors right before their transaction ran, but I guess we should indicate if there was a failure. > 10. Call it something generic like "fs-snapshot" or something, even if > it only works for btrfs atm. > Agreed. > > 99. Man pages FTW ;) -- esp. if you create a config. option. > Sounds good. Thanks for the feedback. Unfortunately my testbox is also my main workstation, which is having other issues at the moment. I will hopefully have this all fixed up and re-sent tomorrow. Thank you, Josef _______________________________________________ Yum-devel mailing list Yum-devel@lists.baseurl.org http://lists.baseurl.org/mailman/listinfo/yum-devel
