[systemd-devel] [RFC][PATCH 1/2] fstab-generator: generate new_root.mount in initrd
Lennart Poettering
lennart at poettering.net
Wed Dec 19 14:56:39 PST 2012
On Wed, 28.11.12 01:30, Tom Gundersen (teg at jklm.no) wrote:
> The configuration is taken from /proc/cmdline, aiming at emulating the
> behavior of the kernel when no initramfs is used.
>
> The supported options are: root=, rootfstype=, rootwait=, rootflags=,
> ro, and rw. rootdelay= was dropped, as it is not really useful in a
> systemd world, but could easily be added.
>
> Cc: Harald Hoyer <harald at redhat.com>
> Cc: Dave Reisner <dreisner at archlinux.org>
> ---
>
>
> Hi guys,
>
> Together with the next patch this aims to add enough kernel commandline parsing support
> to systemd so that it can be used in an initramfs without any extra glue to parse the
> command line and pass on the correct parameters to systemd.
>
> A patch exists using this work to add (shell-free) systemd support to Arch's mkinitcpio [0].
> This is based on similar work in dracut.
>
> Comments welcome,
Sounds good! A few comments:
>
> Tom
>
> [0]: <https://mailman.archlinux.org/pipermail/arch-projects/2012-November/003446.html>
>
>
> src/fstab-generator/fstab-generator.c | 159 +++++++++++++++++++++++++++++-----
> 1 file changed, 135 insertions(+), 24 deletions(-)
>
> diff --git a/src/fstab-generator/fstab-generator.c b/src/fstab-generator/fstab-generator.c
> index ba55f2c..8b0b548 100644
> --- a/src/fstab-generator/fstab-generator.c
> +++ b/src/fstab-generator/fstab-generator.c
> @@ -202,18 +202,21 @@ static bool mount_is_network(struct mntent *me) {
> fstype_is_network(me->mnt_type);
> }
>
> -static int add_mount(const char *what, const char *where, struct mntent *me) {
> +static int add_mount(const char *what, const char *where, const char *type, const char *opts,
> + int passno, bool wait, bool noauto, bool nofail, bool automount, bool isbind, bool isnetwork,
> + const char *source) {
> char *name = NULL, *unit = NULL, *lnk = NULL, *device = NULL, *automount_name = NULL, *automount_unit = NULL;
> FILE *f = NULL;
> - bool noauto, nofail, automount, isbind, isnetwork;
> int r;
> const char *post, *pre;
>
> assert(what);
> assert(where);
> - assert(me);
> + assert(type);
> + assert(opts);
> + assert(source);
>
> - if (streq(me->mnt_type, "autofs"))
> + if (streq(type, "autofs"))
> return 0;
>
> if (!is_path(where)) {
> @@ -225,15 +228,6 @@ static int add_mount(const char *what, const char *where, struct mntent *me) {
> mount_point_ignore(where))
> return 0;
>
> - isnetwork = mount_is_network(me);
> - isbind = !!hasmntopt(me, "bind");
> -
> - noauto = !!hasmntopt(me, "noauto");
> - nofail = !!hasmntopt(me, "nofail");
> - automount =
> - hasmntopt(me, "comment=systemd.automount") ||
> - hasmntopt(me, "x-systemd.automount");
> -
> if (isnetwork) {
> post = SPECIAL_REMOTE_FS_TARGET;
> pre = SPECIAL_REMOTE_FS_PRE_TARGET;
> @@ -264,10 +258,12 @@ static int add_mount(const char *what, const char *where, struct mntent *me) {
> goto finish;
> }
>
> - fputs("# Automatically generated by systemd-fstab-generator\n\n"
> + fprintf(f,
> + "# Automatically generated by systemd-fstab-generator\n\n"
> "[Unit]\n"
> - "SourcePath=/etc/fstab\n"
> - "DefaultDependencies=no\n", f);
> + "SourcePath=%s\n"
> + "DefaultDependencies=no\n",
> + source);
>
> if (!path_equal(where, "/"))
> fprintf(f,
> @@ -293,14 +289,18 @@ static int add_mount(const char *what, const char *where, struct mntent *me) {
> "FsckPassNo=%i\n",
> what,
> where,
> - me->mnt_type,
> - me->mnt_passno);
> + type,
> + passno);
>
> - if (!isempty(me->mnt_opts) &&
> - !streq(me->mnt_opts, "defaults"))
> + if (!isempty(opts) &&
> + !streq(opts, "defaults"))
> fprintf(f,
> "Options=%s\n",
> - me->mnt_opts);
> + opts);
> +
> + if (wait)
> + fprintf(f,
> + "TimeoutSec=0\n");
>
> fflush(f);
> if (ferror(f)) {
> @@ -459,7 +459,13 @@ static int parse_fstab(void) {
> if (streq(me->mnt_type, "swap"))
> k = add_swap(what, me);
> else
> - k = add_mount(what, where, me);
> + k = add_mount(what, where, me->mnt_type, me->mnt_opts,
> + me->mnt_passno, false, !!hasmntopt(me, "noauto"),
> + !!hasmntopt(me, "nofail"),
> + hasmntopt(me, "comment=systemd.automount") ||
> + hasmntopt(me, "x-systemd.automount"),
> + !!hasmntopt(me, "bind"), mount_is_network(me),
> + "/etc/fstab");
Grrh, not an improvement in readability... ;-)
Could you read these ool params into separate bool vars first, which we
then pass (like it was before above)? Makes this a lot more readable.
>
> free(what);
> free(where);
> @@ -473,6 +479,108 @@ finish:
> return r;
> }
>
> +static int parse_new_root_from_proc_cmdline(void) {
> + char *line = NULL, *w, *state, *what = NULL, *type = NULL, *opts = NULL;
> + int r, k;
> + size_t l;
> + bool wait = false;
> +
> + r = read_one_line_file("/proc/cmdline", &line);
> + if (r < 0) {
> + log_warning("Failed to read /proc/cmdline, ignoring: %s", strerror(-r));
> + return 0;
> + }
> +
> + opts = strdup("defaults");
> + if (!opts) {
> + r = log_oom();
> + goto finish;
> + }
> + type = strdup("auto");
> + if (!type) {
> + r = log_oom();
> + goto finish;
> + }
> +
> + /* root= and roofstype= may occur more than once, the last instance should take precedence.
> + * In the case of multiple rootflags= the arguments should be concatenated */
> + FOREACH_WORD_QUOTED(w, l, line, state) {
> + char *word, *tmp_word;
> +
> + word = strndup(w, l);
> + if (!word) {
> + r = log_oom();
> + goto finish;
> + }
> +
> + if (startswith(word, "#")) {
> + break;
This makes no sense I guess if we use this for no-linebreak files such
as /proc/cmdline, which we do.
> +
> + } else if (startswith(word, "root=")) {
> + if (what) {
> + free(what);
> + }
free() deals with NULL pointers just fine, and that is documented by
POSIX even. Hence, please do not check for what != NULL here, just
invoke free() directly, makes things a lot more readable.
> + what = fstab_node_to_udev_node(word+5);
> + if (!what) {
> + r = log_oom();
> + goto finish;
> + }
> +
> + } else if (startswith(word, "rootfstype=")) {
> + if (type)
> + free(type);
Same here...
> + type = strdup(word + 11);
> + if (!type) {
> + r = log_oom();
> + goto finish;
> + }
> +
> + } else if (startswith(word, "rootflags=")) {
> + tmp_word = opts;
> + opts = strjoin(opts, ",", word + 10, NULL);
This doesn't look right: opts is initially empty, hence this would
resolve to strjoin(NULL), i.e. return the empty string...
> + free(tmp_word);
> + if (!opts) {
> + r = log_oom();
> + goto finish;
> + }
> +
> + } else if (streq(word, "ro") || streq(word, "rw")) {
> + tmp_word = opts;
> + opts = strjoin(opts, ",", word, NULL);
Same here...
> + free(tmp_word);
> + if (!opts) {
> + r = log_oom();
> + goto finish;
> + }
> +
> + } else if (streq(word, "rootwait")) {
> + wait = true;
> + }
> +
> + free(word);
> + }
> +
> + if (what) {
> +
> + log_debug("Found entry what=%s where=/new_root type=%s", what, type);
> + k = add_mount(what, "/new_root", type, opts, 0, wait, false, false,
> + false, false, false, "/proc/cmdline");
> + if (k < 0)
> + r = k;
> + }
> +
> +finish:
> + if(what)
> + free(what);
> + if(type)
> + free(type);
> + if(opts)
> + free(opts);
> + if(line)
> + free(line);
Might be good to use _cleanup_free_ for these?
Lennart
--
Lennart Poettering - Red Hat, Inc.
More information about the systemd-devel
mailing list