Hi Jérémy, > Attached you will find an attempt to do so. The changeset is only broke > down in two patches: > * The first is a bit invasive (partman-md, partman-base, mdcfg) and > improve globally the way MD devices are initialized and from my tests > fix 10 bugs at ounce (counting merged ones).
Impressive. Thanks for doing this work :-) Apart from one change which I'm not sure is correct, I couldn't spot any significant problems. More comments below inline with the patches. Note that I haven't actually tested the changes so far but I'm planning to do a few test installs later today. > * The second is in partman-partitioning and prevent deletion and > resizing for MD, LVM and crypto devices as they do not work correctly > for any of these devices. This one is obviously correct. I would actually suggest to commit it straight away, independent of the more invasive changes above. ... > diff --git a/packages/mdcfg/md-init.sh b/packages/mdcfg/md-init.sh > new file mode 100755 > index 0000000..53645f8 > --- /dev/null > +++ b/packages/mdcfg/md-init.sh > @@ -0,0 +1,24 @@ > +#!/bin/sh > + > +if [ -e /proc/mdstat ]; then > + exit 0 > +fi > + > +# Try to load the necesarry modules. (Typo - necessary) > +# Supported schemes: RAID 0, RAID 1, RAID 5 > +depmod -a >/dev/null 2>&1 > +modprobe md >/dev/null 2>&1 || modprobe md-mod >/dev/null 2>&1 > +modprobe raid0 >/dev/null 2>&1 > +modprobe raid1 >/dev/null 2>&1 > +# kernels >=2.6.18 have raid456 > +modprobe raid456 >/dev/null 2>&1 || modprobe raid5 >/dev/null 2>&1 A thought unrelated to your changes since this code is just moved from mdcfg.sh: Should we use something like load_module() { log-output modprobe $1 } load_module md || load_module md-mod .. etc To know when there are problems loading the modules? > diff --git a/packages/partman/partman-base/debian/changelog > b/packages/partman/partman-base/debian/changelog > index 2d63730..31b93af 100644 > --- a/packages/partman/partman-base/debian/changelog > +++ b/packages/partman/partman-base/debian/changelog > @@ -1,3 +1,11 @@ > +partman-base (121) UNRELEASED; urgency=low > + > + [ Jérémy Bobbio ] > + * Instead of skipping every MD devices during partman initialization, we > + know only skip the ones that are deactivated. Typo s/know/now/ ? > + > + -- Jérémy Bobbio <[EMAIL PROTECTED]> Thu, 29 May 2008 16:07:37 +0000 > + > partman-base (120) unstable; urgency=high > > [ Jérémy Bobbio ] > diff --git a/packages/partman/partman-base/init.d/parted > b/packages/partman/partman-base/init.d/parted > index 346170a..f7cfe30 100755 > --- a/packages/partman/partman-base/init.d/parted > +++ b/packages/partman/partman-base/init.d/parted ... > part_of_sataraid () { > local raiddev > for raiddev in $(dmraid -r -c); do > @@ -52,8 +61,7 @@ if [ ! -f /var/run/parted_server.pid ]; then > > IFS="$NL" > for partdev in $(parted_devices | > - grep -v '^/dev/md' | > - sed 's,^/dev/\(ide\|scsi\|[hs]d\),!/dev/\1,' | > + sed 's,^/dev/\(ide\|scsi\|[hsm]d\),!/dev/\1,' | Could be renamed to part_of_raid() or something after the change? > diff --git a/packages/partman/partman-md/debian/changelog > b/packages/partman/partman-md/debian/changelog > index e93fc37..384d64a 100644 > --- a/packages/partman/partman-md/debian/changelog > +++ b/packages/partman/partman-md/debian/changelog > @@ -1,3 +1,16 @@ > +partman-md (42) UNRELEASED; urgency=low > + > + [ Jérémy Bobbio ] > + * Clean up the initialization of MD devices. Together with the changes > + introduced in partman-base (>> 120), setup of RAID devices won't be lost > + accross partman restarts anymore. > + (Closes: #391479, #391483, #393728, #398668) s/accross/across/ > + * Load the necassary modules and scan RAID arrays during partman > + initialization. (Closes: #391474) s/necassary/necessary/ > + Requires mdcfg-utils (>> 1.26). > + > + -- Jérémy Bobbio <[EMAIL PROTECTED]> Thu, 29 May 2008 16:02:39 +0000 > + > partman-md (41) unstable; urgency=low > > [ Frans Pop ] > diff --git a/packages/partman/partman-md/init.d/md > b/packages/partman/partman-md/init.d/md > index e5024f2..96b0b52 100755 > --- a/packages/partman/partman-md/init.d/md > +++ b/packages/partman/partman-md/init.d/md > @@ -2,16 +2,25 @@ > > . /lib/partman/lib/base.sh > > -# Load modules > -#depmod -a 1>/dev/null 2>&1 > -#modprobe md 1>/dev/null 2>&1 || modprobe md-mod 1>/dev/null 2>&1 > -# > -## Load supported personalities > -#modprobe raid1 1>/dev/null 2>&1 > -# > -## Detect and start MD devices > -#/sbin/mdadm --examine --scan --config=partitions >/tmp/mdadm.conf > -#/sbin/mdadm --assemble --scan --run --config=/tmp/mdadm.conf --auto=yes > +# Check if we have RAID > +if [ ! -f /proc/mdstat ]; then > + exit 0 > +fi > + > +# Obtain the size of an MD device > +get_size () { > + while [ -z "$(echo "$line" | grep "^md$NUMBER :")" ]; do > + read line > + [ $? -eq 1 ] && break # EOF > + done > + read line > + size=$(echo "$line" | sed -e 's/ blocks.*//') > + # If the sed failed, the line didn't contain the size; just > + # return 0 in that case. > + if [ "$size" = "$line" ]; then > + size=0 > + fi > +} This function (just moved, I know) is really confusing. After your changes I'm tempted to clean this up to take an explicit parameter and explicitly return size. > # Mark all RAID partitions as being MD > for dev in $DEVICES/*; do > @@ -28,6 +37,7 @@ for dev in $DEVICES/*; do > done > close_dialog > > + # Check if device/partitions are used for software RAID > for id in $partitions; do > md=no > open_dialog GET_FLAGS $id > @@ -43,38 +53,57 @@ for dev in $DEVICES/*; do > fi > done > > - # Next, check if the device is a part of an MD setup > -# if [ -f device ]; then > -# DEVICE=`sed -e "s/\/dev\///" device` > -# grep -q "${DEVICE}" /proc/mdstat > -# if [ $? -eq 0 ]; then > -# open_dialog NEW_LABEL loop > -# close_dialog > -# fi > -# fi > -done > + # Check if the device is a part of an MD setup > + if [ -f device ]; then > + NUMBER=$(sed -e 's,/dev/md/\?,,' device) > + if ! grep -q "^md$NUMBER :" /proc/mdstat; then > + continue > + fi This checks if the device is a RAID device, but the comment suggests that we want to check for it being part of a RAID setup. Is this intended? ... > + # create a label > + open_dialog NEW_LABEL loop > + close_dialog > + # find the free space > + open_dialog PARTITIONS > + free_space='' > + while { read_line num id size type fs path name; [ "$id" ]; }; > do > + if [ "$fs" = free ]; then > + free_space=$id > + free_size=$size > + fi > + done > + close_dialog > + # create partition in the free space > + if [ "$free_space" ]; then > + open_dialog NEW_PARTITION primary ext2 $free_space full > $free_size > + read_line num id size type fs path name > + close_dialog > + if [ "$id" ]; then > + open_dialog GET_FILE_SYSTEM $id > + read_line filesystem > + close_dialog > + if [ "$filesystem" != none ]; then > + open_dialog CHANGE_FILE_SYSTEM $id > $filesystem > + close_dialog > + fi > + fi > + fi > + open_dialog DISK_UNCHANGED > + close_dialog Another thought unrelated to your changes: This code exists in -crypto, -md, -lvm and with a few differences also in partman-auto-lvm. Perhaps we should turn this into a function in -base/lib/. .. <snip> Everything else looks good to me. No problems spotted. ------------ > commit 922e996d3dfa9f545b2e4426d79a5bdcb588fd64 > Author: Jérémy Bobbio <[EMAIL PROTECTED]> > Date: Thu May 29 23:08:16 2008 +0000 > > Disable "resize" and "delete" for partitions on "loop" > > For RAID, crypto and LVM devices only one partition can be handled at a > time. As it makes no sense to resize or delete these kind of partitions, > let's hide the options instead of tricking the users. > > As all these devices are using "loop" partition tables, that's our > filtering criteria here. > > (Closes: #318228, #417973) > > diff --git > a/packages/partman/partman-partitioning/active_partition/delete/choices > b/packages/partman/partman-partitioning/active_partition/delete/choices > index 6587145..5935d7c 100755 > --- a/packages/partman/partman-partitioning/active_partition/delete/choices > +++ b/packages/partman/partman-partitioning/active_partition/delete/choices > @@ -1,7 +1,19 @@ > #!/bin/sh > > -. /usr/share/debconf/confmodule > +. /lib/partman/lib/base.sh > + > +dev=$1 > +id=$2 > +cd $dev > + > +open_dialog GET_LABEL_TYPE > +read_line label > +close_dialog > + > +# Disable on devices where there is no "real" partitioning > +if [ "$label" = loop ]; then > + exit 0 > +fi > > db_metaget partman-partitioning/text/delete description > printf "delete\t${RET}\n" > - > diff --git > a/packages/partman/partman-partitioning/active_partition/resize/choices > b/packages/partman/partman-partitioning/active_partition/resize/choices > index 3b25d94..ef53072 100755 > --- a/packages/partman/partman-partitioning/active_partition/resize/choices > +++ b/packages/partman/partman-partitioning/active_partition/resize/choices > @@ -6,6 +6,15 @@ dev=$1 > id=$2 > cd $dev > > +open_dialog GET_LABEL_TYPE > +read_line label > +close_dialog > + > +# Disable on devices where there is no "real" partitioning > +if [ "$label" = loop ]; then > + exit 0 > +fi > + > if [ -f $id/detected_filesystem ]; then > fs=$(cat $id/detected_filesystem) > case "$fs" in As said above, the idea is obviously correct and the implementation also seems fine to me. :-) Max -- To UNSUBSCRIBE, email to [EMAIL PROTECTED] with a subject of "unsubscribe". Trouble? Contact [EMAIL PROTECTED]