[RFC] Erase LVM/crypto issues and proposed partman reorg - Debian

This is a discussion on [RFC] Erase LVM/crypto issues and proposed partman reorg - Debian ; I've spent about 6 hours yesterday looking at #396023 and #425829, the issues introduced after implementation of support for erasing encrypted volumes. I've looked at both the original patch and the changes proposed by Jérémy. The main issues are that ...

+ Reply to Thread
Results 1 to 5 of 5

Thread: [RFC] Erase LVM/crypto issues and proposed partman reorg

  1. [RFC] Erase LVM/crypto issues and proposed partman reorg

    I've spent about 6 hours yesterday looking at #396023 and #425829, the
    issues introduced after implementation of support for erasing encrypted
    volumes. I've looked at both the original patch and the changes proposed by
    Jérémy.

    The main issues are that the original patch:
    - relies on dmsetup to be loaded basically for all installs
    - introduces a lot of code specific to crypto support in partman-auto
    - introduces new functions with names that can easily be confused with
    existing functions in partman-crypto

    The patch proposed by Jérémy has some good ideas and does improve things to
    some extend, but the end result is still a spaghetti of interrelated
    functions in weird places.
    For example, it moves quite a few lvm/crypto specific functions into
    definitions.sh in partman-base. As definitions.sh is sourced by almost
    every script in partman, I do not think this is desirable.

    I also found that I had real problems following the patches and that
    stacking major corrections on top of David's original changes is not a good
    idea.
    I therefore suggest reverting David's changes (which luckily is quite
    straightforward) and then first do some refactoring of existing code as
    preparation for a reimplementation of support for erasing encrypted
    volumes.

    For refactoring, I propose the following changes.

    1) Rename current "wipe" functions
    Both partman-auto and partman-crypto have wipe functions. In the first case
    it is basically erasing existing data on a disk by creating a new disk
    label; in the second case it is actually wiping the data on a device.
    Using "wipe" for both is confusing.

    I suggest we rename the functions in partman-auto to "erase" or "init"
    instead of "wipe" to better match what actually happens.
    For partman-crypto I have a patch that renames the existing functions to
    include the crypto namespace:
    - wipe -> crypto_do_wipe
    - dev_wipe -> crypto_wipe_device

    2) Reorder function libraries
    We currently have various function libraries: definitions.sh, resize.sh,
    recipes.sh, *_tools.sh.
    - definitions.sh since long contains more than just definitions
    - the various "tools" libraries don't really contain tools
    - in come cases they are starting to contain a somewhat random mix of
    functions and scripts that source them often use only a small subset
    - they all sit in /lib/partman together with the hook dirs and are only
    recognizable because of the .sh extention

    I suggest we move all function libraries into /lib/partman/lib/ [1].
    definitions.sh could be renamed to just base.sh.
    The various *_tools.sh files could be renamed to lvm-base.sh, lvm-*.sh,
    auto-base, etc.

    This would also lower the barrier to introduce additional new function
    libraries when that is warranted.

    For definitions.sh I plan to temporarily include a symlink to the current
    location to ease the transition. For others that is probably not necessary
    if uploads are coordinated.

    3) Further reduce memory consumption for LVM and crypto
    Both partman-lvm and partman-crypto have huge sets of templates and load
    additional modules and library/utility udebs. As they currently are loaded
    by default for normal installs, they contribute heavily to D-I's memory
    footprint.

    I propose we make partman-(auto-){lvm,crypto} optional and create a new
    init.d script that only loads them if there is sufficient memory available.
    This script should probably just be part of partman-base.

    If this is implemented, I also do not have any real problems with depending
    on dmsetup-udeb for both lvm and crypto as it would no longer increase the
    memory usage for systems that cannot afford it. Still, it would be better
    if requiring dmsetup could be avoided for partman-lvm.

    4) Possibly create a partman-dm-support udeb
    From the current patches to support erasing encrypted volumes, it looks like
    partman-lvm and partman-crypto may need to share some code. Moving that
    code into partman-base or partman-auto is IMO undesirable, so the best
    solution seems to be to create a new udeb for it which both depend on.

    Cheers,
    FJP

    [1] I also considered /usr/share/partman, but it seems better to keep things
    together under /lib/partman/. Other idea was to rename all to functions-*.sh
    but moving them to a subdir seems clearer. An alternative could be to use
    /lib/partman/functions/ instead of /lib/partman/lib/.

    -----BEGIN PGP SIGNATURE-----
    Version: GnuPG v1.4.6 (GNU/Linux)

    iD8DBQBHU9nOgm/Kwh6ICoQRAlSvAKC+lI/opz0dIzeZO9g2QPW7vO4YiACgs2C/
    QZFq2VUNRHFYrZNClNS7Ph4=
    =+nbJ
    -----END PGP SIGNATURE-----


  2. Re: [RFC] Erase LVM/crypto issues and proposed partman reorg

    On Mon, Dec 03, 2007 at 11:26:22AM +0100, Frans Pop wrote:
    >I've spent about 6 hours yesterday looking at #396023 and #425829, the
    >issues introduced after implementation of support for erasing encrypted
    >volumes. I've looked at both the original patch and the changes proposed by
    >Jérémy.


    Your suggested changes seem sane and I'm very grateful that you spent
    the time looking into this

    --
    David Härdeman


    --
    To UNSUBSCRIBE, email to debian-boot-REQUEST@lists.debian.org
    with a subject of "unsubscribe". Trouble? Contact listmaster@lists.debian.org

  3. Re: [RFC] Erase LVM/crypto issues and proposed partman reorg

    On Mon, Dec 03, 2007 at 11:26:22AM +0100, Frans Pop wrote:
    > I therefore suggest reverting David's changes (which luckily is quite
    > straightforward) and then first do some refactoring of existing code as
    > preparation for a reimplementation of support for erasing encrypted
    > volumes.


    I tend to agree. The existing code is indeed a bit messy
    and difficult to follow. Starting to untangle the scripts
    and cleaning up the code now seems a good idea.

    David's changes include good code that can be reintroduced
    later on to a large extent when we can start from a more
    maintainable code base.

    > 1) Rename current "wipe" functions


    > For partman-crypto I have a patch that renames the existing functions to
    > include the crypto namespace:
    > - wipe -> crypto_do_wipe
    > - dev_wipe -> crypto_wipe_device


    Good change, agreed. In fact I have a patch sitting here
    that does the exact same change, among others.

    > 2) Reorder function libraries


    > I suggest we move all function libraries into /lib/partman/lib/ [1].
    > definitions.sh could be renamed to just base.sh.
    > The various *_tools.sh files could be renamed to lvm-base.sh, lvm-*.sh,
    > auto-base, etc.
    >
    > This would also lower the barrier to introduce additional new function
    > libraries when that is warranted.


    Yep, this seems worthwile.

    I'm willing to put in some work to help deal with the
    implementation and fallout of this and the other proposed
    changes, (and eventually contribute to the reimplementation
    of the removal of crypto devices). I'm happy to set aside
    some time this weekend and review or test changes.

    Max


    --
    To UNSUBSCRIBE, email to debian-boot-REQUEST@lists.debian.org
    with a subject of "unsubscribe". Trouble? Contact listmaster@lists.debian.org

  4. Re: [RFC] Erase LVM/crypto issues and proposed partman reorg

    On Tuesday 04 December 2007, Max Vozeler wrote:
    > David's changes include good code that can be reintroduced
    > later on to a large extent [...]


    Yes, absolutely. Same goes for the changes suggested by Jérémy.

    I have committed the revert.

    > > 1) Rename current "wipe" functions
    > >
    > > For partman-crypto I have a patch that renames the existing functions
    > > to include the crypto namespace:
    > > - wipe -> crypto_do_wipe
    > > - dev_wipe -> crypto_wipe_device

    >
    > Good change, agreed. In fact I have a patch sitting here
    > that does the exact same change, among others.


    OK. Attached my version of the patch that also includes some other minor
    cleanups. Let me know if I should commit this or that you want to commit
    your own version. However, I will commit my version before I start on the
    reorganization (see below).

    > I'm willing to put in some work to help deal with the
    > implementation and fallout of this and the other proposed
    > changes, (and eventually contribute to the reimplementation
    > of the removal of crypto devices). I'm happy to set aside
    > some time this weekend and review or test changes.


    That's great. I don't expect much fallout, but some extra testing is always
    welcome. And help with the re-implementation is especially welcome.

    I will start work on this tomorrow. If anybody has any pending changes for
    partman (that are solid enough), please commit them before then.


    -----BEGIN PGP SIGNATURE-----
    Version: GnuPG v1.4.6 (GNU/Linux)

    iD8DBQBHVXTEgm/Kwh6ICoQRAlCbAKCSLQQxqp8hDCl2fstE8SJFGSzNiwCdEtLU
    Lg0E/XVX1a5jJ6f6jqXTdl8=
    =POpr
    -----END PGP SIGNATURE-----


  5. Re: [RFC] Erase LVM/crypto issues and proposed partman reorg

    Hi Frans,

    On Tue, Dec 04, 2007 at 04:39:38PM +0100, Frans Pop wrote:
    > > > 1) Rename current "wipe" functions
    > > >
    > > > For partman-crypto I have a patch that renames the existing functions
    > > > to include the crypto namespace:
    > > > - wipe -> crypto_do_wipe
    > > > - dev_wipe -> crypto_wipe_device

    > >
    > > Good change, agreed. In fact I have a patch sitting here
    > > that does the exact same change, among others.

    >
    > OK. Attached my version of the patch that also includes some other minor
    > cleanups. Let me know if I should commit this or that you want to commit
    > your own version. However, I will commit my version before I start on the
    > reorganization (see below).


    Your version seems fine, please commit. Mine is
    dependent on a few other cleanups that should wait until
    after the reorganization.

    > > I'm willing to put in some work to help deal with the
    > > implementation and fallout of this and the other proposed
    > > changes, (and eventually contribute to the reimplementation
    > > of the removal of crypto devices). I'm happy to set aside
    > > some time this weekend and review or test changes.

    >
    > That's great. I don't expect much fallout, but some extra testing is always
    > welcome. And help with the re-implementation is especially welcome.
    >
    > I will start work on this tomorrow. If anybody has any pending changes for
    > partman (that are solid enough), please commit them before then.


    OK. I will monitor the list and commits for anything to do.
    I'll have limited time until friday, but feel free to send
    anything to me directly as well, if you like.

    Max


    --
    To UNSUBSCRIBE, email to debian-boot-REQUEST@lists.debian.org
    with a subject of "unsubscribe". Trouble? Contact listmaster@lists.debian.org

+ Reply to Thread