On Tue, Jul 29, 2008 at 07:42:26PM +0200, Grégory Oestreicher wrote:
> 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.
Please make this a different patch in your patch set.
> > 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.
Yes, that's a lot better. If you could add an example use case for each
of these, it would be even better. ;)
> > 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.
Preseeded encrypted installations are fairly rare: you need to be
sitting right behind the box to type a meaningful passphrase. I think
it does not make a lot of sense to make partman-auto-lvm more complex
that it already is for such corner cases.
> > > + # 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).
I might have overlooked that decode_recipe() is called twice, but I
still think this part would be more at its place in auto_lvm_prepare() as
its extracting informations for the recipe and not yet acting on them.
> > > --- 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.
You are right. But $argdev does not sound too good. What about
$targetdev or something similar?
> > > […]
> > > - 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.
Well, the previous behaviour was encrypting only one device, so it
maked sense to quit the loop as soon as it was found. If you change
this assumption, then code based on it should be removed.
> > 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.
I'm already asking quite some work, but if you have the energy, it would
be lovely. In my humble opinion, it's always harder to read:
+run_foo() {
+ log-output -t foo foo
+}
[…]
-log-output -t foo foo
+if bar; then
+ run_foo
+fi
[…]
- log-output -t foo foo
+ run_foo
[…]
Than:
patch 1: Factor out calls to foo in run_foo()
[…]
+run_foo() {
+ log-output -t foo foo
+}
[…]
-log-output -t foo foo
+run_foo
[…]
- log-output -t foo foo
+ run_foo
[…]
patch 2: Call foo if bar returns true
-run_foo
+if bar; then
+ run_foo
+fi
> > 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.
I am pretty sure that we will manage to find some native speakers to
improve a draft if you write one. :)
Cheers,
--
Jérémy Bobbio .''`.
lunar@debian.org : :Ⓐ : # apt-get install anarchism
`. `'`
`-
Attachment:
signature.asc
Description: Digital signature