Re: [Grml-devel] Grml.org patch bomb
* intrigeri [Thu Jul 28, 2011 at 11:43:13AM +0200]:
I quickly reviewed most of your patches.
Great!
[Just commenting on the ones that need clarification.]
Michael Prokop wrote (26 Jul 2011 21:32:27 GMT) :
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/
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.
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).
25_support_lvm_for_live-media.patch
if [ -x /scripts/local-top/lvm2 ] ; thenif [ -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.
30_support_multiarch_dns.patch
-copy_exec /lib/libnss_dns.so.* /lib # DNS server +# DNS server: +if ls /lib/libnss_dns.so.* >/dev/null 2>&1 ; then # non-multiarch libc
copy_exec /lib/libnss_dns.so.* /lib+elif ls /lib/*/libnss_dns.so.* >/dev/null 2>&1 ; then # multiarch libc
for libnss in /lib/*/libnss_dns.so.* ; docopy_exec "$libnss"
Just curious: wouldn't "copy_exec /lib/*/libnss_dns.so.*" work? Otherwise, this bugfix sounds most welcome to me.
No, since the signature of copy_exec is:
$1 = file to copy to ramdisk $2 (optional) Name for the file on the ramdisk
regards, -mika-
On 07/28/2011 09:01 AM, Michael Prokop wrote:
- intrigeri [Thu Jul 28, 2011 at 11:43:13AM +0200]:
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.
I see these packages are not alone in providing local-top scripts. I gather that to support other local filesystem types provided by additional packages in the archive, each one of these would in turn need to be reviewed and have corresponding patches applied for live-boot to support them (excluding cryptsetup which is already handled, and allowing that some of these may not make sense to support):
$ apt-file search local-top cryptsetup: /usr/share/initramfs-tools/scripts/local-top/cryptopensc cryptsetup: /usr/share/initramfs-tools/scripts/local-top/cryptroot dmraid: /usr/share/initramfs-tools/scripts/local-top/dmraid loop-aes-utils: /usr/share/initramfs-tools/scripts/local-top/loopaes lvm2: /usr/share/initramfs-tools/scripts/local-top/lvm2 mdadm: /usr/share/initramfs-tools/scripts/local-top/mdadm multipath-tools-boot: /usr/share/initramfs-tools/scripts/local-top/multipath nbd-client: /usr/share/initramfs-tools/scripts/local-top/nbd open-iscsi: /usr/share/initramfs-tools/scripts/local-top/iscsi
Not that I'm suggesting we immediately deal with any of these. I ask mostly for my own information trying to understand the code, and to be thorough. :)
Ben
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 ] ; thenif [ -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,
On 08/02/2011 11:21 PM, intrigeri wrote:
=> I'd better add a few kB to the initramfs and get clean, robust and maintainable code.
ack.
Hi,
intrigeri wrote (02 Aug 2011 21:21:17 GMT) :
Michael Prokop wrote (28 Jul 2011 12:01:21 GMT) :
- intrigeri [Thu Jul 28, 2011 at 11:43:13AM +0200]:
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?
Responding to myself: /usr/share/doc/live-boot/copyright will contain copyright info on any system where live-boot is installed, as per Debian Policy 2.3 (Copyright considerations).
=> adapted and reformatted patch, pushed to debian-next (commit add39bd).
Bye,
On 09/08/2011 03:54 PM, intrigeri wrote:
Responding to myself: /usr/share/doc/live-boot/copyright will contain copyright info on any system where live-boot is installed, as per Debian Policy 2.3 (Copyright considerations).
that will not work reliably enough, as some people/derivatives do strip out /usr/share/doc entirely (and we have have hooks for that included in live-build to do that).
=> adapted and reformatted patch, pushed to debian-next (commit add39bd).
updated to use /usr/share/live-boot instead (7ad53e42).
Teilnehmer (4)
-
Ben Armstrong -
Daniel Baumann -
intrigeri -
Michael Prokop