Refactoring commit_changes in partman - Debian

This is a discussion on Refactoring commit_changes in partman - Debian ; Hey all, most of the partman-* packages provide their own slightly different version of commit_changes. I've carefully gone through them and noted the differences, hoping to replace them all with a common commit_changes in partman-base/definition.sh o I plan to apply ...

+ Reply to Thread
Results 1 to 7 of 7

Thread: Refactoring commit_changes in partman

  1. Refactoring commit_changes in partman

    Hey all,

    most of the partman-* packages provide their own slightly
    different version of commit_changes.

    I've carefully gone through them and noted the differences,
    hoping to replace them all with a common commit_changes in
    partman-base/definition.sh

    o I plan to apply the attached path if noone voices
    objections. I've carefully read the code to make sure
    that the changes are safe and given the changed
    packages basic testing.

    o Shrinks the scripts in partman-* by a bit more than
    1k (1257 bytes) in total size.

    o Some error templates were previously shown with high
    priority, while others used critical for essentially
    the same error condition. This change makes them
    consistently use priority critical.

    The only problem I see is that it is not possible to
    add a versioned depends on -base (>= 113) in -partitioning,
    because -base depends on -partitioning and this would
    introduce a circular dependency.

    I'm attaching my more detailed notes in case anyone wants
    to review or double-check.

    Max

    Existing versions:

    script priority exit_commit exit_init
    --------------------------------------------------
    auto_raid critical 1 255
    crypto high 0 255
    dmraid critical 0 255
    lvm critical 0 255
    md high 0 255
    partitioning_1 high 0 0
    partitioning_2 high 100 100
    --------------------------------------------------


    Callers:

    -------------------

    partman-base/partman
    -> partman-auto/display.d/initial_auto
    -> partman-auto-raid/display.d/initial_auto_raid

    Current behaviour:
    commit.d failure -> exit 1
    init.d failure -> exit 255

    ---> No change

    -------------------

    partman-base/partman
    -> partman-base/display.d/manual_partitioning
    -> partman-md/choose_partition/md/do_option
    -> partman-lvm/choose_partition/lvm/do_option
    -> partman-dmread/choose_partition/dmread/do_option
    -> partman-crypto/choose_partition/crypto/do_option

    Current behaviour:
    commit.d failure -> exit 0
    init.d failure -> exit 255

    Called by partman-base/partman in display.d loop
    via partman-base/display.d/manual_partitioning. The
    exit code is propagated all the way up to partman.

    Exception: exit 0 is changed into 99

    (exit 255 causes partman to exit)
    (exit 0 (really 99) causes repeat of display.d loop)

    Treating commit.d as error and exiting 1 means no
    change in behaviour since partman in the display.d
    loop treats all of 1 <= x <= 99 the same way.

    ---> No change in behavour

    -------------------

    partman-base/partman
    -> partman-auto/display.d/initial_auto
    -> partman-auto-crypto/autopartition-crypto
    -> partman-crypt/crypto_tools.sh crypto_setup

    Current behaviour:
    commit.d failure -> exit 0
    init.d failure -> exit 255

    Previously commit.d failure was ignored. Now it
    propagates to initial_auto via autopartition-crypto.
    initial_auto ignores exit code.

    ---> No change

    -------------------

    partman-base/choose_partition/partitiom_tree/do_option
    -> partman-partitioning/active_partition/copy/do_option

    Existing code exits with 0 on any errors.

    Called by partman-base/choose_partition/partition_tree/do_option.
    Any exit code < 100 is ignored. >= 100 stops loop. Error is
    not further propagated.

    --> No change

    -------------------

    partitioning_2: commit in resize.sh

    Existing code exits with 100 on any errors.

    --> No change

    -------------------


  2. Re: Refactoring commit_changes in partman

    On Monday 03 December 2007, Max Vozeler wrote:
    > I've carefully gone through them and noted the differences,
    > hoping to replace them all with a common commit_changes in
    > partman-base/definition.sh


    I agree that factoring this out makes sense. I've looked over your patch and
    the thorough analysis and can't see any holes in it.

    Have you done any testing with the new code? Would be great if you could do
    that before committing.

    > The only problem I see is that it is not possible to
    > add a versioned depends on -base (>= 113) in -partitioning,
    > because -base depends on -partitioning and this would
    > introduce a circular dependency.


    I don't think a circular dependency would do any harm in this case. Suggest
    you just add it.


    Some nitpicks.

    The changelog entry for partman-base could be a bit more elaborate IMO and
    should include an explanation why the function was added.

    I'd also change the changelog entries for the other udebs. Something like:
    * Code to commit changes factored out to new function commit_changes in
    definitions.sh. Requires partman-base (>= 113).

    Cheers,
    FJP

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

    iD8DBQBHU85dgm/Kwh6ICoQRAgwaAKCmA3Gx8aT7EZ4whx4Ygt9boHLXkgCgudci
    aXiNOGt6Z1Z6I2kqPUP0FP8=
    =Dxya
    -----END PGP SIGNATURE-----


  3. Re: Refactoring commit_changes in partman

    Hi Frans,

    On Mon, Dec 03, 2007 at 10:37:32AM +0100, Frans Pop wrote:
    > On Monday 03 December 2007, Max Vozeler wrote:
    > > I've carefully gone through them and noted the differences,
    > > hoping to replace them all with a common commit_changes in
    > > partman-base/definition.sh

    >
    > I agree that factoring this out makes sense. I've looked over your patch and
    > the thorough analysis and can't see any holes in it.
    >
    > Have you done any testing with the new code? Would be great if you could do
    > that before committing.


    Yes, I've tested -auto-crypto, -auto-lvm, -md, -lvm, -crypto
    and -partitioning by doing some random setups and injected the
    two error cases with failing commit.d/init.d scripts.

    I didn't test -dmraid because I couldn't figure out how, :-)
    Does dmraid require special hardware?

    > > The only problem I see is that it is not possible to
    > > add a versioned depends on -base (>= 113) in -partitioning,
    > > because -base depends on -partitioning and this would
    > > introduce a circular dependency.

    >
    > I don't think a circular dependency would do any harm in this case.
    > Suggest you just add it.


    Tried this and ended up with a "Deep recursion configuring
    partman-base (dep loop?)" (IIRC) from main-menu. It seems
    that main-menu tries to configure partman-base and all its
    dependencies, and this fails.

    > Some nitpicks.
    >
    > The changelog entry for partman-base could be a bit more elaborate IMO and
    > should include an explanation why the function was added.


    Agreed, I'll try to write something more of an explanation.

    Thanks for your review.

    Max


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

  4. Re: Refactoring commit_changes in partman

    On Tuesday 04 December 2007, Max Vozeler wrote:
    > Yes, I've tested -auto-crypto, -auto-lvm, -md, -lvm, -crypto
    > and -partitioning by doing some random setups and injected the
    > two error cases with failing commit.d/init.d scripts.


    Perfect.

    > I didn't test -dmraid because I couldn't figure out how, :-)
    > Does dmraid require special hardware?


    Well, the hardware is fairly common nowadays, but yes, it does require a
    somewhat special setup.
    See http://wiki.debian.org/DebianInstaller/SataRaid for info about
    partman-dmraid.

    > Tried this and ended up with a "Deep recursion configuring
    > partman-base (dep loop?)" (IIRC) from main-menu. It seems
    > that main-menu tries to configure partman-base and all its
    > dependencies, and this fails.


    OK. Wasn't aware main-menu would choke on this. In that case just the
    mention in the changelog is sufficient.

    Cheers,
    FJP

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

    iD8DBQBHVXTMgm/Kwh6ICoQRAjR+AKCo6nwsOfcuBqFa5ommQyrlpVybewCfXN3g
    SB+CmZuvGUhyIhZtYuXixPI=
    =twjR
    -----END PGP SIGNATURE-----


  5. Re: Refactoring commit_changes in partman

    On Tuesday 04 December 2007, Frans Pop wrote:
    > On Tuesday 04 December 2007, Max Vozeler wrote:
    > > Yes, I've tested -auto-crypto, -auto-lvm, -md, -lvm, -crypto
    > > and -partitioning by doing some random setups and injected the
    > > two error cases with failing commit.d/init.d scripts.

    >
    > Perfect.


    And please commit today so it's in before I start the restructuring.

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

    iD8DBQBHVXTTgm/Kwh6ICoQRAnXtAJ9DOsiMQEmiQ7QjaIIipk6j+K5N3wCeL18b
    cNDyBSZqL6XITKLRam7Bex8=
    =cKRd
    -----END PGP SIGNATURE-----


  6. Re: Refactoring commit_changes in partman

    On Tue, Dec 04, 2007 at 04:40:03PM +0100, Frans Pop wrote:
    > On Tuesday 04 December 2007, Frans Pop wrote:
    > > On Tuesday 04 December 2007, Max Vozeler wrote:
    > > > Yes, I've tested -auto-crypto, -auto-lvm, -md, -lvm, -crypto
    > > > and -partitioning by doing some random setups and injected the
    > > > two error cases with failing commit.d/init.d scripts.

    > >
    > > Perfect.

    >
    > And please commit today so it's in before I start the restructuring.


    OK, I've commited the changes.

    I think it would be useful to do a quick round of
    uploads before starting the reorganization, so that we
    have a known baseline to compare to and can tell any
    problems resulting from this change.

    What do you think? If you agree, could you do them
    before starting reorga, or should I do them tonight?
    (I probably won't have time for it tomorrow).

    Max


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

  7. Re: Refactoring commit_changes in partman

    On Tuesday 04 December 2007, Max Vozeler wrote:
    > > And please commit today so it's in before I start the restructuring.

    >
    > OK, I've commited the changes.


    Thanks Max.

    > I think it would be useful to do a quick round of
    > uploads before starting the reorganization, so that we
    > have a known baseline to compare to and can tell any
    > problems resulting from this change.


    Agreed. We have enough real pending changes to make that worthwhile and it
    would be good to have a version with LVM removal regressions fixed in the
    archive.

    > What do you think? If you agree, could you do them
    > before starting reorga, or should I do them tonight?
    > (I probably won't have time for it tomorrow).


    I'll do some basic testing with current versions and upload before I start
    the reorganization.

    Cheers,
    FJP

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

    iD8DBQBHVcgNgm/Kwh6ICoQRAm8mAJ9QXGnzBpOjT44M0lXQSKsYqcB0KgCcCT/i
    0CiEQWfgowHaHX0/e6rU7IY=
    =uV4n
    -----END PGP SIGNATURE-----


+ Reply to Thread