[Yum-devel] [PATCH] add the btrfs-snapshot plugin to yum-utils

James Antill james at fedoraproject.org
Thu Dec 10 05:37:00 UTC 2009


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.

4. Accessing ts.po.dirlist for a "remote" package means the user needs
to download the filelist MD. This isn't easy to fix :(.

5. You don't take bind() mounts into account

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.

9. Does it output anything on the error path?

10. Call it something generic like "fs-snapshot" or something, even if
it only works for btrfs atm.


99. Man pages FTW ;) -- esp. if you create a config. option.



More information about the Yum-devel mailing list