[Buildroot] [PATCH 4/5] fs/custom: generate complete, partition-based device images
Yann E. MORIN
yann.morin.1998 at free.fr
Mon Nov 25 19:05:51 UTC 2013
Arnout,
On 2013-11-25 10:31 +0100, Arnout Vandecappelle spake thusly:
> This patch is obviously too large to be reviewed in a single shot, so here
> are some detailed comments on certain parts of it.
Yes, I did expect it to be not trivial. Thank you for trying! :-)
> I don't think there's a way to split the patch up to make review easier,
> unfortunately. But anyway, the functionality is completely isolated from the
> rest so it doesn't hurt to commit it as is and fix up later if necessary.
Maybe I could have separated the doc changes from the actual
implementation, to make it easier to review, and eventuall squash
both in a single cset for the final suvmission.
> On 22/11/13 23:50, Yann E. MORIN wrote:
> >From: "Yann E. MORIN" <yann.morin.1998 at free.fr>
> >Contrary to the existing fs/ schemes, which each generate only a single
> >filesystem image for the root filesystem, this new scheme allows the
> >user to generate more complex images.
> >diff --git a/fs/Config.in b/fs/Config.in
> >index da4c5ff..8721220 100644
> >--- a/fs/Config.in
> >+++ b/fs/Config.in
> >@@ -11,5 +11,6 @@ source "fs/romfs/Config.in"
> > source "fs/squashfs/Config.in"
> > source "fs/tar/Config.in"
> > source "fs/ubifs/Config.in"
> >+source "fs/custom/Config.in"
>
> Shouldn't this be kept alphabetical?
I also wondered about it, but my reasoning was to leave all single-fs
options grouped by themselves, and add this new one as the last (or
first) in the menu, to explicitly differentiate it.
But I have no stronger opinion than "I find it nicer".
> >
> > endmenu
> >diff --git a/fs/custom/Config.in b/fs/custom/Config.in
> >new file mode 100644
> >index 0000000..fabb878
> >--- /dev/null
> >+++ b/fs/custom/Config.in
> >@@ -0,0 +1,18 @@
> >+config BR2_TARGET_ROOTFS_CUSTOM_PARTITION_TABLE
> >+ string "partition table"
>
> In most cases, we don't rely on the non-emptiness of a string to determine
> if some option is enabled or not, but rather there's an explicit config
> option to enable it. I'm not convinced that that is a good principle, but
> it's how things are done now.
OK, I see.
My reasoning is that passing the path to the "partition table" is enough
to state that the user does want to use a partition table, hence I did
not hide it behind a bool.
That's what we do for (eg.) BR2_ROOTFS_DEVICE_TABLE: if it is empty, we
treat it as to not add any device. The fact that the option is set/unset
is in itself enough to act or not.
The boolean below is indeed needed since we do need a boolean for our
internal use, but I see no reason to hide the string option behind the
boolean. Hence the boolean is a blind option.
> >+ help
> >+ Enter the path to a partition-table for your device.
> >+
> >+ This will allow Buildroot to generate a more complex target
> >+ image, which may consist of more than one filesystem on more
> >+ than one partition.
> >+
> >+ See docs/manual/bla-bla on how to construct such a partition
>
> I couldn't find the bla-bla section :-)
Hey! :-)
> We currently have one reference to the manual, and it's formatted like
> this:
>
> http://buildroot.org/downloads/manual/manual.html#rootfs-custom
OK, I'll update once the doc has stabilised.
> I like that much better - though it's too long of course, but perhaps Peter
> can create a shortlink http://buildroot.org/man that expands to
> http://buildroot.org/downloads/manual/manual.html ?
Yep, that would be nice.
> >+ table.
> >+
> >+# For the fs infrasturcture, we need that option to be set.
> >+# Additionally, it allows us to force the TAR output.
> >+config BR2_TARGET_ROOTFS_CUSTOM
> >+ def_bool y
> >+ depends on BR2_TARGET_ROOTFS_CUSTOM_PARTITION_TABLE != ""
>
> In most other cases we use the following construct:
>
> bool
> default y if BR2_TARGET_ROOTFS_CUSTOM_PARTITION_TABLE != ""
OK, I'll change.
Note however that the two constructs are not equivalent in kconfig (but
for us they give the same result). One is about default value and
depdency of the symbol (mine), the other is about the dependency of a
default value (yours).
> >+ select BR2_TARGET_ROOTFS_TAR
>
> [snip, unreviewed]
>
> >diff --git a/fs/custom/custom.mk b/fs/custom/custom.mk
> >new file mode 100644
> >index 0000000..940a32a
> >--- /dev/null
> >+++ b/fs/custom/custom.mk
> >@@ -0,0 +1,14 @@
> >+################################################################################
> >+#
> >+# custom partitioning
> >+#
> >+################################################################################
> >+
> >+define ROOTFS_CUSTOM_CMD
> >+ BUILD_DIR=$(BUILD_DIR) fs/custom/genimages \"$(BR2_TARGET_ROOTFS_CUSTOM_PARTITION_TABLE)\"
>
> Should be indented with a tab.
No, because it is used as:
echo "$$(ROOTFS_$(2)_CMD)" >> $$(FAKEROOT_SCRIPT)
> Does that quoting work? It sill expand to
Yes it does, because it is interpreted twice (not sure how, but if I
remove the quotes and set BR2_TARGET_ROOTFS_CUSTOM_PARTITION_TABLE="foo bar"
then genimages gets two args, not one. Don;t ask me, double indirection
in the Makefile fragemnt plus shell expansion, something like it...
> genimages \""path/to/partitiontable"\"
> so genimages' $1 will be "path/to/partititiontable" including the quotes.
No, because both quotes have already been stripped away.
>
> Anyway, the symbol should be qstrip'ped and any required quotes added
> explicitly. That makes it easier to run
>
> make BR2_TARGET_ROOTFS_CUSTOM_PARTITION_TABLE=some/other/path
So, qstrip the symbol and re-add the quotes? Tss... OK, I can do that if
you want. ;-)
> >+endef
> >+
> >+# We need the tarball to be generated first
>
> This comment is redundant
What about:
# rootfs-custom uses rootfs.tar as the source to generate the
# resulting image(s), so we need to build it first.
Also, I forgot to add:
rootfs-custom-show-depends:
@echo $(ROOTFS_CUSTOM_DEPENDENCIES)
> >+ROOTFS_CUSTOM_DEPENDENCIES += rootfs-tar
> >+
> >+$(eval $(call ROOTFS_TARGET,custom))
>
> [snip, unreviewed]
>
> >diff --git a/fs/custom/genimages b/fs/custom/genimages
> >new file mode 100755
> >index 0000000..aba3021
> >--- /dev/null
> >+++ b/fs/custom/genimages
> >@@ -0,0 +1,214 @@
> >+#!/bin/bash
>
> I have a general question for the maintainers here:
>
> * Do we really want to rely on bash even more?
bash is already a hard-dependency of Buildroot. anyway.
> * Would it make sense to implement complex things like this in a proper
> programming language (read: python), which would solidify our dependency on
> python?
I don't do Python.
> With python's ConfigParser, this file would be reduced to +- 20 lines...
Does ConfigParser handles lines like:
key=$((4*1024))
> >+set -e
> >+set -E
> >+
> >+#-----------------------------------------------------------------------------
> >+main() {
> >+ local part_table="${1}"
> >+ local tmp_dir
> >+ local rootfs_dir
> >+ local -a devices
> >+ local extract
> >+ local cur_section
> >+ local -a sections devices partitions
> >+ local -A variables values partdevs
> >+ local sec dev part var val
> >+ local secs devs parts vars vals
> >+
> >+ if [ ! -f "${part_table}" ]; then
> >+ error "%s: no such file\n" "${part_table}"
> >+ exit 1
> >+ fi
> >+
> >+ export PATH="${HOST_DIR}/usr/bin:${HOST_DIR}/usr/sbin:${PATH}"
> >+
> >+ tmp_dir="$( mktemp -d --tmpdir rpi-genimage.XXXXXX )"
>
> Small nit: I think it would make more sense to create the tmp_dir relative
> to the output directory.
OK.
> Larger nit: there should be a trap to clean up tmp_dir. Or alternatively,
> if it's relative to the output directory do the cleanup in the beginning.
I initially had that. But since we may want to debug the issues in
genimages, we need to keep the temp dir. So, moving to build-dir and
cleaning at the beginning is OK for my use-case.
> And finally, I'd create the tmp_dir only after parsing and validating the
> partition file.
OK.
> >+ rootfs_dir="${tmp_dir}/rootfs"
> >+ mkdir -p "${rootfs_dir}"
> >+
> >+ # Parse all the sections in one go, we'll sort
> >+ # all the mess afterwards...
> >+ debug "parsing partitions descriptions file '%s'\n" \
> >+ "${part_table}"
> >+ while read line; do
> >+ line="$( sed -r -e 's/[[:space:]]*#.*$//; //d;' <<<"${line}" )"
>
> I would try to avoid sed -r if you don't really need it - especially if you
> don't use extended regexp at all.
I never care about regexp being extended or not, I just use -r all the
time. It is much simpler: I never know if the regexp I'm using is
extended or not.
> To make this incredibly complex line a little more readable, I'd split it
> in two lines:
>
> line="$( sed -e 's/[[:space:]]*#.*$//' <<<"${line}" )"
> line="$( sed -e '//d;' <<<"${line}" )"
"Incredibly complex"? You're kidding, aren't you? Besides, yours is
broken, since '//d;' relies on the previous expresion. ;-p
> >+
> >+ # Detect start of global section, skip anything else
> >+ case "${line}" in
> >+ "") continue;;
> >+ '['*']')
> >+ cur_section="$( sed -r -e 's/[][]//g;' <<<"${line}" )"
> >+ debug " entering section '%s'\n" "${cur_section}"
> >+ sections+=( "${cur_section}" )
> >+ continue
> >+ ;;
> >+ ?*=*) ;;
> >+ *) error "malformed entry '%s'\n" "${line}";;
> >+ esac
> >+
> >+ var="${line%%=*}"
> >+ eval val="${line#*=}"
> >+ debug " adding '%s'='%s'\n" "${var}" "${val}"
> >+ variables+=( ["${cur_section}"]=",${var}" )
> >+ values+=( ["${cur_section}:${var}"]="${val}" )
> >+ done <"${part_table}"
>
> I would add explicit checks that the global section exists and that it
> includes the required variables.
OK, makes sense. Ditto, all referenced devices/partitions should have a
corresponding section.
> >+ # Create lists of devices, partitions, and partition:device pairs.
> >+ debug "creating intermeditate lists\n"
> >+ devices=( ${values["global:devices"]//,/ } )
> >+ for dev in "${devices[@]}"; do
> >+ partitions=( ${values["${dev}:partitions"]//,/ } )
Hmm. Bug here -------^ should be += I think.
> I'd include a check that partitions exsits and has the right format.
See above.
> >+ done
> >+ for dev in "${devices[@]}"; do
> >+ for part in ${values["${dev}:partitions"]//,/ }; do
> >+ partdevs+=( ["${part}"]="${dev}" )
>
> Why do you loop twice here, instead of just doing this in the previous
> loop?
Hmm, lemme check...
> I'm not very familiar with bash array syntax, but can't you use something
> like "for part in ${partitions[@]}" ?
But how do I know what device a partition is on?
(but do I need that anyway?) I'll check.
> >+ trace "extracting root (if needed)\n"
> >+ case "${values["global:extract"]}" in
> >+ targz) c=z ;;&
> >+ tarbz2) c=j ;;&
> >+ tarxz) c=J ;;&
>
> Since we use a fairly recent tar, tar will auto-detect compression based on
> the extension.
I don't like that... But OK.
> By the way, since you only support tar anyway, I would remove
> this option completely. Whenever it is actually useful, it can be added
> again (with a default to tar for backwards compatibility).
Yep, no need to over-engineer the sutff.
> >+ tar) c= ;;&
> >+ tar*) tar x${c}f "${BINARIES_DIR}/rootfs.tar" -C "${rootfs_dir}";;
> >+ "") ;;
> >+ *) error "unknown extract method '%s'\n" "${extract}";;
> >+ esac
>
> [Rest is unreviewed]
Thank you very much for the review! :-)
Regards,
Yann E. MORIN.
--
.-----------------.--------------------.------------------.--------------------.
| Yann E. MORIN | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software Designer | \ / CAMPAIGN | ___ |
| +33 223 225 172 `------------.-------: X AGAINST | \e/ There is no |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL | v conspiracy. |
'------------------------------^-------^------------------^--------------------'
More information about the buildroot
mailing list