Bug#462396: Multiple disks support for partman-auto-lvm
Le mardi 29 juillet 2008 02:18, Jérémy Bobbio a écrit :
> > +# 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.
OK, I'll move this into auto-shared.sh and submit an updated patch later this
week.
> > + 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?
I don't think writing a function to perform a db_metaget and a printf will be
necessary. The humandev() function also tries to get the exact type of disk
which is not needed here, so the translation can be done inline.
> > @@ -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.
Here it is :
We need to create the envelope only if one is not defined. This is the case
when :
- the default device is not part of a PV declaration in the scheme (a PV
is declared when there's a method{ lvm } or method{ crypto } attribute).
- the recipe contains a PV declaration *without* device. For this case the
physical device used will be the default one.
If it's OK with you (and clear) I'll integrate this in the next patch.
> 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?
I think you missed the '-v' in the last grep of the second test. When a device
is not specified, as it is the case with the default recipes, the PV will be
created on the default device. It will also be integrated in the default VG.
>
> > […]
> > 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.
I did this to allow to use the name of the encrypted device in the recipes.
Now that you point it I don't think it's really useful, it was more a
convenience. I'll remove it from future versions.
However the same kind of checks / removal is done further down in the file,
and these, despite being a convenience, are still useful : they allow people
to use "normal" name of the device (ie /dev/hda2) instead of the
partman-crypto name (/dev/mapper/hda2_crypt) in the recipes.
> > + # 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?
decode_recipe() is called twice : once in auto_lvm_prepare, and once in
auto_lvm_perform. This is needed to have $scheme in scope for further checks.
Before the call to decode_recipe, $scheme is not visible in the current scope
(I don't think it still exists at all in fact).
> > […]
> > --- 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
OK, will remove this.
> > --- 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?
Yes, $dev is overwritten by the next for loop and I need it untouched as first
argument to auto_lvm_perform. I thought there were less risks of side effects
to change the name of this variable here rather than changing all $dev
occurences in the for loop.
> > […]
> > - 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.
I hesitated before leaving it commented out in the public patch, but decided
to go anyway like this to have advices from people who know why these breaks
are here. It's true I should have added a comment asking. I couldn't see any
problem with commenting this out, as all devices must be encrypted when there
are more than one, but once again, it needs to be approved.
> On the overall, parts of this patch makes me uneasy. Maybe because
> refactoring is mixed up with functional changes…
Some minor refactoring was needed when writing the code, so why not doing it
while it's still hot ? However I can submit two separate patches if it's
clearer.
> 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?
Well, if the fact that I'm not so fluent in english is not a major drawback I
can give it a try. Are there guidelines to write documentation for Debian,
such as the style to use and so on (I saw something like this, but can't
remember if it was for Debian or another project) ?
Cheers,
Grégory
Reply to: