Hi,
anonym wrote (20 Mar 2013 14:16:31 GMT) :
> New commits in bugfix/writable_boot_media:
> ae530a1 New fix for bugs/writable_system_disk:_belongs_to_floppy_group
> 05c8cf6 Add a shell library for determining stuff about the boot device.
Fixing t-p-s and the reasons behind its need for calling sgdisk is on
it's way, but let's go parallel and do a first review pass on this
bugfix/writable_boot_media branch.
First, I do like the general solution you've implemented, and the fact
it can be used to improve other, unrelated areas of Tails. Thanks!
,----
| /usr/local/lib/tails-shell-library/boot.sh
`----
First, I'm very happy someone eventually tackles this. I expect this
will be useful for other bits of code.
However, I'm a bit confused, as it looks to me like this library is
either duplicating, or re-implementing from scratch, some code that we
have had in /usr/local/sbin/udev-watchdog-wrapper for a while... that
takes into account corner cases such as fromiso. Perhaps they should
be merged or something?
> +# Note dependency on working udev
Well, this is not true of functions called by udev-boot-dev-helper, is
it? If I'm correct, given we have in here functions that depend on
udev working, and other functions that absolutely should not rely or
udev nor call udevadm, I suggest they are split into two different
files, to avoid confusion and programming mistakes.
,----
| /etc/udev/rules.d/99-boot-dev-ownership.rules
`----
I'd rather see this file called 99-boot-dev-ownership.rules, for
consistency with 91-permissions.rules, and also because at some point
we might want to do other permissions trickery than ownership on these
devices (ACL, Unix permissions, etc.), and I'd rather split the rules
according to the high-level goal rather than according to the
low-level tool being used to achieve it.
> +# Fix for Debian bug #645466.
Minor nitpicking: referencing external sources is great for details
lovers, but IMHO it should not replace telling the basics of what this
file is supposed to do. Not a blocker, anyway.
,----
| /usr/local/sbin/udev-boot-dev-helper
`----
s/an udev rule/a udev rule/
> +# Turns out we cannot use function using `udevadm` in this library for
> +# this script since it's used in an udev rule; at that time the udev
> +# database isn't finished and any queries in it cannot be trusted.
> +. /usr/local/lib/tails-shell-library/boot.sh
Then perhaps add a note, on top of every such library function that's
being used, to make it clear it should *never* use udevadm? (If you
don't go with the "split into two files" way.)
> +# XXX: This code is pretty crude thanks to not having udev to query
> +# for the parent device. In Wheezy with its newer blkid we'll be able
> +# to determine the parent device more reliably, if we care.
Good news this can be improved later.
Would you please move this note to todo/Wheezy (and perhaps point to
there from a code comment if you wish)? It's been a while (if ever)
that I've seen someone tackle a FIXME or XXX that's in our code.
Let's not spread our TODO list to places nobody looks at :)
> grep -q "^/dev/sd[a-z]$"
Single quotes would be safer here.
Also, in such calls to grep, generally you want `-qs'.
Same for the next one.
To end with, once polished a bit, I think this should be tested (if
not done yet):
* on DVD
* on a SD card with a USB controller
Bonus points if this is tested too:
* with fromiso (yeah, well, we don't support this officially)
* on mmc_block SD cards whose name is along the lines of
/dev/mmcblk0 (these are not supported yet by our installer etc.)
Cheers,
--
intrigeri
| GnuPG key @ https://gaffer.ptitcanardnoir.org/intrigeri/intrigeri.asc
| OTR fingerprint @ https://gaffer.ptitcanardnoir.org/intrigeri/otr.asc
