On Mon, Jul 28, 2008 at 06:54:12PM +0200, Grégory Oestreicher wrote:
> Here is an updated version of the patch, dealing with LVM+crypto, as
> suggested by Jérémy Bobbio. This patch also incorporate his modifications
> to add the possibility to define a custom LV name (see
> http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=484272).
A few comments after a quick review:
> Index: debian/partman-auto-lvm.templates
> ===================================================================
> --- debian/partman-auto-lvm.templates (révision 54599)
> +++ debian/partman-auto-lvm.templates (copie de travail)
> @@ -42,3 +42,19 @@
> the volume group.
> .
> Check /var/log/syslog or see virtual console 4 for the details.
> +
> +Template: partman-auto-lvm/no_such_pv
> +Type: error
> +_Description: Inexistant PV declared
> + A Volume Group definition contains a reference to a Physical Volume
> + that is not present.
> + .
> + Check for you connectivity or the recipe.
> +
> +Template: partman-auto-lvm/no_pv_in_vg
> +Type: error
> +_Description: No PV defined for creating VG
> + The recipe contains the definition of a Volume Group without
> + any Physical Volume in it.
> + .
> + Review the recipe.
These surely will need improvements by native speakers.
> Index: lib/auto-lvm.sh
> ===================================================================
> --- lib/auto-lvm.sh (révision 54599)
> +++ lib/auto-lvm.sh (copie de travail)
> @@ -10,14 +10,48 @@
> exit 1
> }
>
> +# This function was copied from another file (partman-auto/lib/initial_auto).
> +# Maybe it should be useful to put it in auto-shared.sh or in base.sh
> +dev_to_partman () {
> […]
It is already in partman-auto-raid, so I think it should be factored in
auto-shared.sh.
> auto_lvm_prepare() {
> - local dev method size free_size normalscheme target
> + local dev method size free_size normalscheme target extra_devices tmpdev tmpdevdir physdev
> […]
> - target="$(humandev $(cat $dev/device)) - $(cat $dev/model)"
> + if [ $extra_devices ]; then
> + for extradev in $dev $extra_devices; do
> + extradev_str="$extradev_str $(cat $extradev/device)"
> + done
> + target="Multiple disks ($extradev_str)"
> + else
> + target="$(humandev $(cat $dev/device)) - $(cat $dev/model)"
> + fi
"Multiple disks" should be translatable. Maybe factored out in a
humandevs() function?
> @@ -101,27 +141,70 @@
> # (still one atm)
> devfspv_devices=''
>
> - create_primary_partitions
> + # Creating envelope
> + # Only if one does not already exist (identified by 'method{ lvm }' and by
> + # the current device in the scheme)
> + physdev=$(cat $dev/device)
> + if ! echo "$normalscheme" | grep -E "method\{ (lvm|crypto) \}" | grep -q "device{ $physdev[[:digit:]]* }" && \
> + ! echo "$normalscheme" | grep -E "method\{ (lvm|crypto) \}" | grep -q -v "device{"; then
> + normalscheme="$normalscheme${NL}100 1000 1000000000 ext3 \$primary{ } method{ $method }"
> + fi
The comment should probably be expanded a little bit to mention the use
cases. Something like:
Creating envelope:
(This is only done for recipes where the location of the Physical
Volumes have not been already defined, as the default ones.)
Why the test is actually made twice? Isn't "method{ lvm }" always
defining a physical volume? All your test recipes have a
"device{ ... }" defined, what happens when it is not specified?
> […]
> auto_lvm_perform() {
> + local IFS physdev defvgname schemeline schemevg schemedev targetdir targetvg pvdevice
> + [ -n $1 ] || return 1
> +
> + physdev=$(cat $1/device)
> + if echo $physdev | grep -q '/mapper/.*_crypt'; then
> + physdev=$(echo $physdev | sed -re 's#mapper/([^[:digit:]]+)[[:digit:]]_crypt#\1#')
> + fi
This makes me uneasy: optional partman components tries to stay pretty
independant from each others and the use of "_crypt" here breaks this
assumption. I don't get exactly how is this useful, either.
> + # The recipe contains all the necessary informations about eventuals
> + # extra VGs to create
> + # The VGs to create are :
> + # - the default one if some partitions don't have the invg{ } tag
> + # - the ones present in vgname{ } tags of PVs
> + decode_recipe $recipe lvm
> […]
Is there anything preventing this step to happen in auto_lvm_prepare
where the recipe is already available?
> […]
> --- perform_recipe_by_lvm (révision 54599)
> +++ perform_recipe_by_lvm (copie de travail)
> […]
> decode_recipe $recipe lvm
> +tmpscheme=$(echo "$scheme" | grep lvmok)
> +scheme=$(echo "$tmpscheme" | grep "invg{ *$VG_name *}")
> +if [ $VG_name = $default_vgname ]; then
> + # Adding in the scheme the partitions that have no VG declared
> + extraschemelines=$(echo "$tmpscheme" | grep lvmok | grep -v 'invg{')
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
lvmok has already been filtered out here
> + scheme="${scheme:+$scheme$NL}$extraschemelines"
> +fi
Otherwise, this looks convoluted, but I have been unable to find
something better…
> […]
> --- autopartition-crypto (révision 54599)
> +++ autopartition-crypto (copie de travail)
> @@ -3,10 +3,10 @@
> . /lib/partman/lib/auto-lvm.sh
> . /lib/partman/lib/crypto-base.sh
>
> -dev="$1"
> +argdev="$1"
> method=crypto
>
> -auto_lvm_prepare $dev $method || exit 1
> +auto_lvm_prepare $argdev $method || exit 1
Looks like a gratious change. Any rationale?
> […]
> - found=yes
> - break
> +# found=yes
> +# break
> done
> - [ $found = yes ] && break
> +# [ $found = yes ] && break
Commented out code without any explaination is a major hassle. If it
should not be there anymore, please delete it and explain why.
On the overall, parts of this patch makes me uneasy. Maybe because
refactoring is mixed up with functional changes…
An important thing missing from your patch is user documentation.
Actually there is currently nothing in manual/en/appendix/preseed.xml
concerning LVM recipes. Do you feel like sketching a new part about it?
Cheers,
--
Jérémy Bobbio .''`.
lunar@debian.org : :Ⓐ : # apt-get install anarchism
`. `'`
`-
Attachment:
signature.asc
Description: Digital signature