On Wed, 28.11.12 01:30, Tom Gundersen ([email protected]) 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 <[email protected]> > Cc: Dave Reisner <[email protected]> > --- > > > 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. _______________________________________________ systemd-devel mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/systemd-devel
