Re: [systemd-devel] [RFC] link against util-linux for fstab parsing
On Mon, Feb 20, 2012 at 09:50:54AM -0500, Dave Reisner wrote:
> Well, it does get shorter if you keep going with it (there's code in
> umount that can be replaced). replacing the mountinfo sscanf parsing
> with libmount seems like more of a win to me. My working tree is
> currently looking like +172/-219 and replaces code in
> src/{,u}mount.c and src/cryptsetup/cryptsetup-generator.c
>
> Rewriting remount-api-vfs purely with libmount almost works, but then
> doesn't if you want to be able to fork for each mount call. Karel, maybe
> I missed something... mnt_context_next_mount() doesn't seem to let you
> filter in the way sd's mount_point_is_api() does (filtering on target),
Currently it's impossible, it uses fstype and options patterns only
(it's backend for "umount -a" and "mount -a"). But we can extend for
example libmnt_context or libmnt_iter to call some external filter
function.
> but you can't fork internally for mount(2) if you don't use that call.
Yes, maybe the fork() stuff should be moved into mnt_context_mount()
and then it will be available without mnt_context_next_mount()
machinery. I'll try to improve it for the next release.
> Perhaps Karel can be a better salesman than I can. =P
It seems I missed the opportunity to buy a few beers to Lennart last week ;-)
> > Dave, Karel, can you fill in any reasons here? If not I think we
> > shouldn't apply this patch...
This is Dave's activity, but I'm happy that someone wants to use the
library. (So thanks Dave!)
I have talked with Lennart about libmount for systemd on IRC few
months ago and I know about the fork(2) requirement (and the
requirement makes a lot of sense). So the libmount for systemd was
not my primary goal, it's more long-term wish ;-)
Anyway, number of removed lines should not be the real reason. We're
slowly moving important low-level system stuff to the shared
libraries to avoid duplicate code and consolidate functionality.
BTW, for the next libmount release (2.22) I'd like to support
swapon(2) and swapoff(2) in the library.
Karel
--
Karel Zak
http://karelzak.blogspot.com
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [RFC] link against util-linux for fstab parsing
On Mon, 20.02.12 09:50, Dave Reisner (d...@falconindy.com) wrote:
>
> On Mon, Feb 20, 2012 at 03:31:01PM +0100, Lennart Poettering wrote:
> > On Fri, 17.02.12 16:47, Dave Reisner (d...@falconindy.com) wrote:
> >
> > > Based on the premise that we shouldn't develop a case of NIH, link
> > > against a library whose sole purpose in life is parsing tab files.
> >
> > Hmmm, using the glibc api setmntent() is hardly NIH, is it?
> >
> > I am not strictly against this, but I'd like to have good reasons for
> > this. Right now I see on the side against this:
> >
> > - adds another dependency
> > - code isn't really any shorter than right now
> >
> > On the pro side:
>
> Well, it does get shorter if you keep going with it (there's code in
> umount that can be replaced). replacing the mountinfo sscanf parsing
> with libmount seems like more of a win to me. My working tree is
> currently looking like +172/-219 and replaces code in
> src/{,u}mount.c and src/cryptsetup/cryptsetup-generator.c
OK, this sounds not too bad. But I need more on the "pro" side.
> Rewriting remount-api-vfs purely with libmount almost works, but then
> doesn't if you want to be able to fork for each mount call. Karel, maybe
> I missed something... mnt_context_next_mount() doesn't seem to let you
> filter in the way sd's mount_point_is_api() does (filtering on target),
> but you can't fork internally for mount(2) if you don't use that call.
I'd prefer keeping the mounts out of process here. (Why? well, NSS calls
make me suspicious, like they might be required for the gid=xxx param
for devtmpfs, and suchlike).
> > > Curious if something like this is wanted -- it's 90% complete, but
> > > there's no
> > > sense in finishing it up if it's not interesting. I'd like to be able to
> > > get
> > > rid of the fstab_node_to_udev_node() function but that would likely
> > > require
> > > linking against libblkid in cryptsetup-generator.
> >
> > I am not sure about this? What's wrong with fstab_node_to_udev_node()?
>
> Nothing's wrong with it. I was a bit hasty in assuming it could be
> ditched -- the value of it is that it always creates a "resolved" path
> for LABEL and UUID tags without actually checking for existance. Trying
> to resolve LABEL=foo via mnt_resolve_spec() for a crypto volume before
> its unlocked of course leads to Bad Things, as I found out in testing.
Yes, we need to be able to resolve LABEL= and UUID= without actually
having the disks arounds.
Lennart
--
Lennart Poettering - Red Hat, Inc.
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [RFC] link against util-linux for fstab parsing
On Mon, Feb 20, 2012 at 03:31:01PM +0100, Lennart Poettering wrote:
> On Fri, 17.02.12 16:47, Dave Reisner (d...@falconindy.com) wrote:
>
> > Based on the premise that we shouldn't develop a case of NIH, link
> > against a library whose sole purpose in life is parsing tab files.
>
> Hmmm, using the glibc api setmntent() is hardly NIH, is it?
>
> I am not strictly against this, but I'd like to have good reasons for
> this. Right now I see on the side against this:
>
> - adds another dependency
> - code isn't really any shorter than right now
>
> On the pro side:
Well, it does get shorter if you keep going with it (there's code in
umount that can be replaced). replacing the mountinfo sscanf parsing
with libmount seems like more of a win to me. My working tree is
currently looking like +172/-219 and replaces code in
src/{,u}mount.c and src/cryptsetup/cryptsetup-generator.c
Rewriting remount-api-vfs purely with libmount almost works, but then
doesn't if you want to be able to fork for each mount call. Karel, maybe
I missed something... mnt_context_next_mount() doesn't seem to let you
filter in the way sd's mount_point_is_api() does (filtering on target),
but you can't fork internally for mount(2) if you don't use that call.
Perhaps Karel can be a better salesman than I can. =P
> Dave, Karel, can you fill in any reasons here? If not I think we
> shouldn't apply this patch...
>
> > Curious if something like this is wanted -- it's 90% complete, but there's
> > no
> > sense in finishing it up if it's not interesting. I'd like to be able to get
> > rid of the fstab_node_to_udev_node() function but that would likely require
> > linking against libblkid in cryptsetup-generator.
>
> I am not sure about this? What's wrong with fstab_node_to_udev_node()?
Nothing's wrong with it. I was a bit hasty in assuming it could be
ditched -- the value of it is that it always creates a "resolved" path
for LABEL and UUID tags without actually checking for existance. Trying
to resolve LABEL=foo via mnt_resolve_spec() for a crypto volume before
its unlocked of course leads to Bad Things, as I found out in testing.
Was a fun experiment, but if all we can really get out of this is
replacing the parsers, maybe this isn't worth it.
d
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [RFC] link against util-linux for fstab parsing
On Fri, 17.02.12 16:47, Dave Reisner (d...@falconindy.com) wrote: > Based on the premise that we shouldn't develop a case of NIH, link > against a library whose sole purpose in life is parsing tab files. Hmmm, using the glibc api setmntent() is hardly NIH, is it? I am not strictly against this, but I'd like to have good reasons for this. Right now I see on the side against this: - adds another dependency - code isn't really any shorter than right now On the pro side: - ?? Dave, Karel, can you fill in any reasons here? If not I think we shouldn't apply this patch... > Curious if something like this is wanted -- it's 90% complete, but there's no > sense in finishing it up if it's not interesting. I'd like to be able to get > rid of the fstab_node_to_udev_node() function but that would likely require > linking against libblkid in cryptsetup-generator. I am not sure about this? What's wrong with fstab_node_to_udev_node()? Lennart -- Lennart Poettering - Red Hat, Inc. ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [RFC] link against util-linux for fstab parsing
On Fri, Feb 17, 2012 at 04:47:51PM -0500, Dave Reisner wrote:
> +tb = mnt_new_table();
> +if (!tb) {
> +return -ENOMEM;
> +}
>
> -while ((me = getmntent(f))) {
> -char *where, *what;
> -int k;
> +mnt_cache = mnt_new_cache();
> +if (!mnt_cache) {
> +r = -ENOMEM;
> +goto finish;
> +}
The cache is completely unnecessary here as you don't search in the
table (fstab).
>
> -if (!(what = fstab_node_to_udev_node(me->mnt_fsname))) {
> -r = -ENOMEM;
> -goto finish;
> -}
> +mnt_table_set_cache(tb, mnt_cache);
>
> -if (!(where = strdup(me->mnt_dir))) {
> -free(what);
> -r = -ENOMEM;
> -goto finish;
> -}
> +r = mnt_table_parse_fstab(tb, "/etc/fstab");
Please, use mnt_table_parse_fstab(tb, NULL); The filename is there to
overwrite library default.
> finish:
> +if (tb)
> +mnt_free_table(tb);
> +if (mnt_cache)
> +mnt_free_cache(mnt_cache);
like libc free() all mnt_free_* functions accept NULL.
--
Karel Zak
http://karelzak.blogspot.com
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [RFC] link against util-linux for fstab parsing
On Fri, Feb 17, 2012 at 11:41:05PM +0100, Michael Biebl wrote: > Am 17. Februar 2012 22:47 schrieb Dave Reisner : > > Makefile.am | 6 ++- > > configure.ac | 1 + > > src/mount.c | 157 > > +- > > 3 files changed, 83 insertions(+), 81 deletions(-) > > Interesting numbers. By using the external library we actually need > more code in systemd itself. > > So I'm wondering what the benefits of using the external library are. > > Michael > -- > Why is it that all of the instruments seeking intelligent life in the > universe are pointed away from Earth? Hmmm, if I finish out this patch, it ends up being more along the lines of +87/-163 deletions. There's an awful lot of duplicate code in cryptsetup-generator (e.g. has_option() is completely redundant). d ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [RFC] link against util-linux for fstab parsing
Am 17. Februar 2012 22:47 schrieb Dave Reisner : > Makefile.am | 6 ++- > configure.ac | 1 + > src/mount.c | 157 > +- > 3 files changed, 83 insertions(+), 81 deletions(-) Interesting numbers. By using the external library we actually need more code in systemd itself. So I'm wondering what the benefits of using the external library are. Michael -- Why is it that all of the instruments seeking intelligent life in the universe are pointed away from Earth? ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
[systemd-devel] [RFC] link against util-linux for fstab parsing
Based on the premise that we shouldn't develop a case of NIH, link
against a library whose sole purpose in life is parsing tab files.
---
Curious if something like this is wanted -- it's 90% complete, but there's no
sense in finishing it up if it's not interesting. I'd like to be able to get
rid of the fstab_node_to_udev_node() function but that would likely require
linking against libblkid in cryptsetup-generator.
Worth noting:
* in mount_fix_timeouts(), i've removed the possibility of using
'comment=systemd.device-timeout'. This option is undocumented in
systemd.mount(5), and util-linux will not parse out the value properly.
* is that this won't compile without util-linux from git or one of
the recent RCs as it depends on mnt_fs_is_swaparea().
Makefile.am |6 ++-
configure.ac |1 +
src/mount.c | 157 +-
3 files changed, 83 insertions(+), 81 deletions(-)
diff --git a/Makefile.am b/Makefile.am
index 10c85a4..2559cde 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -493,7 +493,8 @@ libsystemd_core_la_CFLAGS = \
$(LIBWRAP_CFLAGS) \
$(PAM_CFLAGS) \
$(AUDIT_CFLAGS) \
- $(KMOD_CFLAGS)
+ $(KMOD_CFLAGS) \
+ $(MOUNT_CFLAGS)
libsystemd_core_la_LIBADD = \
libsystemd-basic.la \
@@ -503,7 +504,8 @@ libsystemd_core_la_LIBADD = \
$(PAM_LIBS) \
$(AUDIT_LIBS) \
$(CAP_LIBS) \
- $(KMOD_LIBS)
+ $(KMOD_LIBS) \
+ $(MOUNT_LIBS)
# This is needed because automake is buggy in how it generates the
# rules for C programs, but not Vala programs. We therefore can't
diff --git a/configure.ac b/configure.ac
index 62e8cdf..c8a8440 100644
--- a/configure.ac
+++ b/configure.ac
@@ -126,6 +126,7 @@ m4_pattern_forbid([^_?PKG_[A-Z_]+$],[*** pkg.m4 missing,
please install pkg-conf
PKG_CHECK_MODULES(UDEV, [ libudev >= 172 ])
PKG_CHECK_MODULES(DBUS, [ dbus-1 >= 1.3.2 ])
PKG_CHECK_MODULES(KMOD, [ libkmod >= 5 ])
+PKG_CHECK_MODULES(MOUNT, [ mount ])
have_selinux=no
AC_ARG_ENABLE(selinux, AS_HELP_STRING([--disable-selinux], [Disable optional
SELINUX support]))
diff --git a/src/mount.c b/src/mount.c
index 5e52a54..cc675d4 100644
--- a/src/mount.c
+++ b/src/mount.c
@@ -25,6 +25,8 @@
#include
#include
+#include
+
#include "unit.h"
#include "mount.h"
#include "load-fragment.h"
@@ -262,27 +264,10 @@ static int mount_add_socket_links(Mount *m) {
return 0;
}
-static char* mount_test_option(const char *haystack, const char *needle) {
-struct mntent me;
-
-assert(needle);
-
-/* Like glibc's hasmntopt(), but works on a string, not a
- * struct mntent */
-
-if (!haystack)
-return false;
-
-zero(me);
-me.mnt_opts = (char*) haystack;
-
-return hasmntopt(&me, needle);
-}
-
static bool mount_is_network(MountParameters *p) {
assert(p);
-if (mount_test_option(p->options, "_netdev"))
+if (mnt_match_options(p->options, "_netdev"))
return true;
if (p->fstype && fstype_is_network(p->fstype))
@@ -294,7 +279,7 @@ static bool mount_is_network(MountParameters *p) {
static bool mount_is_bind(MountParameters *p) {
assert(p);
-if (mount_test_option(p->options, "bind"))
+if (mnt_match_options(p->options, "bind"))
return true;
if (p->fstype && streq(p->fstype, "bind"))
@@ -312,11 +297,11 @@ static bool needs_quota(MountParameters *p) {
if (mount_is_bind(p))
return false;
-return mount_test_option(p->options, "usrquota") ||
-mount_test_option(p->options, "grpquota") ||
-mount_test_option(p->options, "quota") ||
-mount_test_option(p->options, "usrjquota") ||
-mount_test_option(p->options, "grpjquota");
+return mnt_match_options(p->options, "usrquota") ||
+ mnt_match_options(p->options, "grpquota") ||
+ mnt_match_options(p->options, "quota") ||
+ mnt_match_options(p->options, "usrjquota") ||
+ mnt_match_options(p->options, "grpjquota");
}
static int mount_add_fstab_links(Mount *m) {
@@ -337,15 +322,15 @@ static int mount_add_fstab_links(Mount *m) {
if (p != &m->parameters_etc_fstab)
return 0;
-noauto = !!mount_test_option(p->options, "noauto");
-nofail = !!mount_test_option(p->options, "nofail");
+noauto = mnt_match_options(p->options, "+noauto");
+nofail = mnt_match_options(p->options, "+nofail");
automount =
-mount_test_option(p->options, "comment=systemd.automount") ||
-mount_test_option(p->options, "x-systemd-automount");
+mnt_match_options(p->options, "comment=systemd.automount") ||
+mnt_match_options(p->options, "x-systemd-automount");
handle =
automount ||
-
