[Grml-devel] Grml.org patch bomb

[intrigeri]

Hi,

Michael Prokop wrote (28 Jul 2011 12:01:21 GMT) :
> * intrigeri [Thu Jul 28, 2011 at 11:43:13AM +0200]:

>> >  -			umount ${mountpoint}
>> >  +			umount ${mountpoint} 2>/dev/null

>> Rationale for this seemingly unrelated change?
>> I fear it could make debugging harder.

> It's to not show an error message if live/image has been already
> unmounted before. For the context of the code see
> http://paste.pocoo.org/show/448076/

Fair enough => ack.

>> >  +		grep -q /live/findiso /proc/mounts && umount /root/live/findiso

>> The grep command could be a bit more specific, and hence more robust
>> I believe. Isn't it possible to use the mountpoint command here?

> This would mean that we'd have to add mountpoint(1) to the initramfs
> using copy_exec, hmmm.

/bin/mountpoint is 6,6K on my Wheezy system.
According to ldd, the only lib it depends on is libc, which is
probably already in the initramfs.
So I don't think it's a big deal.

Else, "awk '{print $2}' < /proc/mounts | ..." could be used, but
frankly, I'm not sure adding more text parsing to the mix would add
any robustness at all.

=> I'd better add a few kB to the initramfs and get clean, robust and
   maintainable code.

>> > 10_validateroot.patch

>> I like defensive programming, but I fear this one might serve to hide
>> a problem you encountered with scripts/live not dealing with some kind
>> of particular case. Is this live-bottom script here just in case,
>> or...?

> If live-boot finds a "wrong" file system that looks OK for
> live-boot then the error message can be pretty confusing.
> Though I don't like the check for /sbin/init neither, so
> feel free to ignore this patch (and we'd investigate on that
> further).

Ok. I see the need for it, and I may see why you don't like it.
Is there any file any system running live-boot has, and any other
system hasn't?

>> > 25_support_lvm_for_live-media.patch

>> +			if [ -x /scripts/local-top/lvm2 ] ; then
>> +			if [ -x /scripts/local-top/mdadm ] ; then

>> I'm doubtful about adding support for (if I understand clearly)
>> GRML-specific local-* scripts that are not shipped in live-boot.

> They aren't Grml specific local scripts, they are shipped by the
> original lvm2 and mdadm Debian packages.

Oops, sorry. I'll look at this one again then.

Unless someone else has merged, in the meantime, the patches we've
reached an agreement upon, I'll do it myself in a few weeks. Feel free
to ping me if I seem to forget.

Bye,
-- 
  intrigeri <intrigeri at boum.org>
  | GnuPG key @ https://gaffer.ptitcanardnoir.org/intrigeri/intrigeri.asc
  | OTR fingerprint @ https://gaffer.ptitcanardnoir.org/intrigeri/otr.asc
  | So what?