| Message ID | 156712723849.29242.14842855181957166256.stgit@mickey.themaw.net |
|---|---|
| State | New |
| Headers | show |
| Series | glibc: use autofs mount hint in getmntent_r() | expand |
* Ian Kent: > Historically autofs mounts were not included in mount table > listings. This is the case in other SysV autofs implementations > and was also the case with Linux autofs. > > But now that /etc/mtab is a symlink to the proc filesystem > mount table the autofs mount entries appear in the mount table > on Linux. Overall, this looks good. Thanks. I added a ChangeLog entry, tweaked the return type of get_mnt_entry to bool, and avoided the parameter assignment in __getmntent_r. Do you think these changes are okay? I'll post my new test separately. Thanks, Florian From: Ian Kent <ikent@redhat.com> Use autofs "ignore" mount hint in getmntent_r/getmntent Historically autofs mounts were not included in mount table listings. This is the case in other SysV autofs implementations and was also the case with Linux autofs. But now that /etc/mtab is a symlink to the proc filesystem mount table the autofs mount entries appear in the mount table on Linux. Prior to the symlinking of /etc/mtab mount table it was sufficient to call mount(2) and simply not update /etc/mtab to exclude autofs mounts from mount listings. Also, with the symlinking of /etc/mtab we have seen a shift in usage toward using the proc mount tables directly. But the autofs mount entries need to be retained when coming from the proc file system for applications that need them (largely autofs file system users themselves) so filtering out these entries within the kernel itself can't be done. So it needs be done in user space. There are three reasons to omit the autofs mount entries. One is that certain types of auto-mounts have an autofs mount for every entry in their autofs mount map and these maps can be quite large. This leads to mount table listings containing a lot of unnecessary entries. Also, this change in behaviour between autofs implementations can cause problems for applications that use getmntent(3) in other OS implementations as well as Linux. Lastly, there's very little that user space can do with autofs mount entries since this must be left to the autofs mount owner, typically the automount daemon. But it can also lead to attempts to access automount managed paths resulting mounts being triggered when they aren't needed or mounts staying mounted for much longer thay they need be. While the point of this change ins't to help with these problems (and it can be quite a problem) it may be a welcome side effect. So the Linux autofs file system has been modified to accept a pseudo mount option of "ignore" (as is used in other OS implementations) so that user space can use this as a hint to skip autofs entries on reading the mount table. The Linux autofs automount daemon used getmntent(3) itself and has been modified to use the proc file system directly so that it can "ignore" mount option. The use of this mount option is opt-in and a configuration option has been added which defaults to not use this option so if there are applications that need these entries, other than autofs itself, they can be retained. Also, since this filtering is based on an added mount option earlier versions of Linux autofs iand other autofs file system users will not use the option and so won't be affected by the change. 2019-09-02 Ian Kent <ikent@redhat.com> Use autofs "ignore" mount hint in getmntent_r/getmntent. * misc/mntent_r.c (get_mnt_entry): New function, extracted from getmntent_r. (__getmntent_r): Call it. Filter out autofs entries with an "ignore" mount option. diff --git a/misc/mntent_r.c b/misc/mntent_r.c index 5d88c45c6f..d90e8d7087 100644 --- a/misc/mntent_r.c +++ b/misc/mntent_r.c @@ -18,6 +18,7 @@ #include <alloca.h> #include <mntent.h> +#include <stdbool.h> #include <stdio.h> #include <stdio_ext.h> #include <string.h> @@ -112,26 +113,18 @@ decode_name (char *buf) return buf; } - -/* Read one mount table entry from STREAM. Returns a pointer to storage - reused on the next call, or null for EOF or error (use feof/ferror to - check). */ -struct mntent * -__getmntent_r (FILE *stream, struct mntent *mp, char *buffer, int bufsiz) +static bool +get_mnt_entry (FILE *stream, struct mntent *mp, char *buffer, int bufsiz) { char *cp; char *head; - flockfile (stream); do { char *end_ptr; if (__fgets_unlocked (buffer, bufsiz, stream) == NULL) - { - funlockfile (stream); - return NULL; - } + return false; end_ptr = strchr (buffer, '\n'); if (end_ptr != NULL) /* chop newline */ @@ -181,9 +174,40 @@ __getmntent_r (FILE *stream, struct mntent *mp, char *buffer, int bufsiz) case 2: break; } + + return true; +} + +/* Read one mount table entry from STREAM. Returns a pointer to storage + reused on the next call, or null for EOF or error (use feof/ferror to + check). */ +struct mntent * +__getmntent_r (FILE *stream, struct mntent *mp, char *buffer, int bufsiz) +{ + struct mntent *result; + + flockfile (stream); + while (true) + if (get_mnt_entry (stream, mp, buffer, bufsiz)) + { + /* If the file system is autofs look for a mount option hint + ("ignore") to skip the entry. */ + if (strcmp (mp->mnt_type, "autofs") == 0 && __hasmntopt (mp, "ignore")) + memset (mp, 0, sizeof (*mp)); + else + { + result = mp; + break; + } + } + else + { + result = NULL; + break; + } funlockfile (stream); - return mp; + return result; } libc_hidden_def (__getmntent_r) weak_alias (__getmntent_r, getmntent_r)
And here is the new test.
Thanks,
Florian
Add misc/tst-mntent-autofs, testing autofs "ignore" filtering
2019-09-02 Florian Weimer <fweimer@redhat.com>
* misc/tst-mntent-autofs.c: New file.
* misc/Makefile (tests): Add misc/tst-mntent-autofs.
diff --git a/misc/Makefile b/misc/Makefile
index 032f28fc38..afb8b023e8 100644
--- a/misc/Makefile
+++ b/misc/Makefile
@@ -86,7 +86,8 @@ tests := tst-dirname tst-tsearch tst-fdset tst-mntent tst-hsearch \
tst-mntent-blank-corrupt tst-mntent-blank-passno bug18240 \
tst-preadvwritev tst-preadvwritev64 tst-makedev tst-empty \
tst-preadvwritev2 tst-preadvwritev64v2 tst-warn-wide \
- tst-ldbl-warn tst-ldbl-error tst-dbl-efgcvt tst-ldbl-efgcvt
+ tst-ldbl-warn tst-ldbl-error tst-dbl-efgcvt tst-ldbl-efgcvt \
+ tst-mntent-autofs
# Tests which need libdl.
ifeq (yes,$(build-shared))
diff --git a/misc/tst-mntent-autofs.c b/misc/tst-mntent-autofs.c
new file mode 100644
index 0000000000..bf4d4e73b4
--- /dev/null
+++ b/misc/tst-mntent-autofs.c
@@ -0,0 +1,141 @@
+/* Test autofs "ignore" filtering for getment_r.
+ Copyright (C) 2019 Free Software Foundation, Inc.
+ This file is part of the GNU C Library.
+
+ The GNU C Library is free software; you can redistribute it and/or
+ modify it under the terms of the GNU Lesser General Public
+ License as published by the Free Software Foundation; either
+ version 2.1 of the License, or (at your option) any later version.
+
+ The GNU C Library is distributed in the hope that it will be useful,
+ but WITHOUT ANY WARRANTY; without even the implied warranty of
+ MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ Lesser General Public License for more details.
+
+ You should have received a copy of the GNU Lesser General Public
+ License along with the GNU C Library; if not, see
+ <http://www.gnu.org/licenses/>. */
+
+#include <array_length.h>
+#include <errno.h>
+#include <mntent.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+#include <support/check.h>
+#include <support/temp_file.h>
+#include <support/xstdio.h>
+#include <support/xunistd.h>
+
+struct test_case
+{
+ const char *line;
+ struct
+ {
+ /* Like struct mntent, but with const pointers. */
+ const char *mnt_fsname;
+ const char *mnt_dir;
+ const char *mnt_type;
+ const char *mnt_opts;
+ int mnt_freq;
+ int mnt_passno;
+ } expected;
+};
+
+static struct test_case test_cases[] =
+ {
+ { "/etc/auto.direct /mnt/auto/1 autofs defaults 0 0",
+ { "/etc/auto.direct", "/mnt/auto/1", "autofs", "defaults", 0, 0 } },
+
+ /* These entries are filtered out. */
+ { "/etc/auto.2 /mnt/auto/2 autofs ignore 0 0", { NULL, } },
+ { "/etc/auto.3 /mnt/auto/3 autofs ignore,other 1 2", { NULL, } },
+ { "/etc/auto.4 /mnt/auto/4 autofs other,ignore 3 4", { NULL, } },
+ { "/etc/auto.5 /mnt/auto/5 autofs opt1,ignore,opt2 5 6", { NULL, } },
+
+ /* Dummy entry to make the desynchronization more obvious. */
+ { "/dev/sda1 / xfs defaults 0 0",
+ { "/dev/sda1", "/", "xfs", "defaults", 0, 0 } },
+
+ /* These are not filtered because the file system is not autofs. */
+ { "/etc/auto.direct /mnt/auto/6 autofs1 ignore 0 0",
+ { "/etc/auto.direct", "/mnt/auto/6", "autofs1", "ignore", 0, 0 } },
+ { "/etc/auto.direct /mnt/auto/7 autofs1 ignore,other 0 0",
+ { "/etc/auto.direct", "/mnt/auto/7", "autofs1", "ignore,other", 0, 0 } },
+ { "/etc/auto.direct /mnt/auto/8 autofs1 other,ignore 0 0",
+ { "/etc/auto.direct", "/mnt/auto/8", "autofs1", "other,ignore", 0, 0 } },
+ { "/etc/auto.direct /mnt/auto/9 autofs1 opt1,ignore,opt2 0 0",
+ { "/etc/auto.direct", "/mnt/auto/9", "autofs1", "opt1,ignore,opt2", } },
+
+ /* These are not filtered because the string "ignore" is not an
+ option name. */
+ { "/etc/auto.direct /mnt/auto/10 autofs noignore 1 2",
+ { "/etc/auto.direct", "/mnt/auto/10", "autofs", "noignore", 1, 2 } },
+ { "/etc/auto.direct /mnt/auto/11 autofs noignore,other 0 0",
+ { "/etc/auto.direct", "/mnt/auto/11", "autofs", "noignore,other", } },
+ { "/etc/auto.direct /mnt/auto/12 autofs other,noignore 0 0",
+ { "/etc/auto.direct", "/mnt/auto/12", "autofs", "other,noignore", } },
+ { "/etc/auto.direct /mnt/auto/13 autofs errors=ignore 0 0",
+ { "/etc/auto.direct", "/mnt/auto/13", "autofs", "errors=ignore", } },
+ { "/etc/auto.direct /mnt/auto/14 autofs errors=ignore,other 0 0",
+ { "/etc/auto.direct", "/mnt/auto/14", "autofs",
+ "errors=ignore,other", } },
+ { "/etc/auto.direct /mnt/auto/15 autofs other,errors=ignore 0 0",
+ { "/etc/auto.direct", "/mnt/auto/15", "autofs",
+ "other,errors=ignore", } },
+
+ /* These are not filtered because the string is escaped. '\151'
+ is 'i', but it is not actually decoded by the parser. */
+ { "/etc/auto.\\151gnore /mnt/auto/16 autofs \\151gnore 0 0",
+ { "/etc/auto.\\151gnore", "/mnt/auto/16", "autofs",
+ "\\151gnore", } },
+ };
+
+static int
+do_test (void)
+{
+ char *path;
+ xclose (create_temp_file ("tst-mntent-autofs-", &path));
+
+ /* Write the test file. */
+ FILE *fp = xfopen (path, "w");
+ for (size_t i = 0; i < array_length (test_cases); ++i)
+ fprintf (fp, "%s\n", test_cases[i].line);
+ xfclose (fp);
+
+ /* Open the test file again, this time for parsing. */
+ fp = setmntent (path, "r");
+ TEST_VERIFY_EXIT (fp != NULL);
+ char buffer[512];
+ struct mntent me;
+
+ for (size_t i = 0; i < array_length (test_cases); ++i)
+ {
+ if (test_cases[i].expected.mnt_type == NULL)
+ continue;
+
+ memset (buffer, 0xcc, sizeof (buffer));
+ memset (&me, 0xcc, sizeof (me));
+ struct mntent *pme = getmntent_r (fp, &me, buffer, sizeof (buffer));
+ TEST_VERIFY_EXIT (pme != NULL);
+ TEST_VERIFY (pme == &me);
+ TEST_COMPARE_STRING (test_cases[i].expected.mnt_fsname, me.mnt_fsname);
+ TEST_COMPARE_STRING (test_cases[i].expected.mnt_dir, me.mnt_dir);
+ TEST_COMPARE_STRING (test_cases[i].expected.mnt_type, me.mnt_type);
+ TEST_COMPARE_STRING (test_cases[i].expected.mnt_opts, me.mnt_opts);
+ TEST_COMPARE (test_cases[i].expected.mnt_freq, me.mnt_freq);
+ TEST_COMPARE (test_cases[i].expected.mnt_passno, me.mnt_passno);
+ }
+
+ TEST_VERIFY (getmntent_r (fp, &me, buffer, sizeof (buffer)) == NULL);
+
+ TEST_COMPARE (feof (fp), 1);
+ TEST_COMPARE (ferror (fp), 0);
+ errno = 0;
+ TEST_COMPARE (endmntent (fp), 1);
+ TEST_COMPARE (errno, 0);
+ free (path);
+ return 0;
+}
+
+#include <support/test-driver.c>
On Mon, 2019-09-02 at 13:34 +0200, Florian Weimer wrote: > * Ian Kent: > > > Historically autofs mounts were not included in mount table > > listings. This is the case in other SysV autofs implementations > > and was also the case with Linux autofs. > > > > But now that /etc/mtab is a symlink to the proc filesystem > > mount table the autofs mount entries appear in the mount table > > on Linux. > > Overall, this looks good. Thanks. > > I added a ChangeLog entry, tweaked the return type of get_mnt_entry > to > bool, and avoided the parameter assignment in __getmntent_r. Do you > think these changes are okay? The changes look fine to me, thanks for them and the changelog entry. > > I'll post my new test separately. > > Thanks, > Florian > > From: Ian Kent <ikent@redhat.com> > > Use autofs "ignore" mount hint in getmntent_r/getmntent > > Historically autofs mounts were not included in mount table > listings. This is the case in other SysV autofs implementations > and was also the case with Linux autofs. > > But now that /etc/mtab is a symlink to the proc filesystem > mount table the autofs mount entries appear in the mount table > on Linux. > > Prior to the symlinking of /etc/mtab mount table it was > sufficient to call mount(2) and simply not update /etc/mtab > to exclude autofs mounts from mount listings. > > Also, with the symlinking of /etc/mtab we have seen a shift in > usage toward using the proc mount tables directly. > > But the autofs mount entries need to be retained when coming > from the proc file system for applications that need them > (largely autofs file system users themselves) so filtering out > these entries within the kernel itself can't be done. So it > needs be done in user space. > > There are three reasons to omit the autofs mount entries. > > One is that certain types of auto-mounts have an autofs mount > for every entry in their autofs mount map and these maps can > be quite large. This leads to mount table listings containing > a lot of unnecessary entries. > > Also, this change in behaviour between autofs implementations > can cause problems for applications that use getmntent(3) in > other OS implementations as well as Linux. > > Lastly, there's very little that user space can do with autofs > mount entries since this must be left to the autofs mount owner, > typically the automount daemon. But it can also lead to attempts > to access automount managed paths resulting mounts being triggered > when they aren't needed or mounts staying mounted for much longer > thay they need be. While the point of this change ins't to help > with these problems (and it can be quite a problem) it may be > a welcome side effect. > > So the Linux autofs file system has been modified to accept a > pseudo mount option of "ignore" (as is used in other OS > implementations) so that user space can use this as a hint to > skip autofs entries on reading the mount table. > > The Linux autofs automount daemon used getmntent(3) itself and > has been modified to use the proc file system directly so that > it can "ignore" mount option. > > The use of this mount option is opt-in and a configuration > option has been added which defaults to not use this option > so if there are applications that need these entries, other > than autofs itself, they can be retained. Also, since this > filtering is based on an added mount option earlier versions > of Linux autofs iand other autofs file system users will not > use the option and so won't be affected by the change. > > 2019-09-02 Ian Kent <ikent@redhat.com> > > Use autofs "ignore" mount hint in getmntent_r/getmntent. > * misc/mntent_r.c (get_mnt_entry): New function, extracted from > getmntent_r. > (__getmntent_r): Call it. Filter out autofs entries with an > "ignore" mount option. > > diff --git a/misc/mntent_r.c b/misc/mntent_r.c > index 5d88c45c6f..d90e8d7087 100644 > --- a/misc/mntent_r.c > +++ b/misc/mntent_r.c > @@ -18,6 +18,7 @@ > > #include <alloca.h> > #include <mntent.h> > +#include <stdbool.h> > #include <stdio.h> > #include <stdio_ext.h> > #include <string.h> > @@ -112,26 +113,18 @@ decode_name (char *buf) > return buf; > } > > - > -/* Read one mount table entry from STREAM. Returns a pointer to > storage > - reused on the next call, or null for EOF or error (use > feof/ferror to > - check). */ > -struct mntent * > -__getmntent_r (FILE *stream, struct mntent *mp, char *buffer, int > bufsiz) > +static bool > +get_mnt_entry (FILE *stream, struct mntent *mp, char *buffer, int > bufsiz) > { > char *cp; > char *head; > > - flockfile (stream); > do > { > char *end_ptr; > > if (__fgets_unlocked (buffer, bufsiz, stream) == NULL) > - { > - funlockfile (stream); > - return NULL; > - } > + return false; > > end_ptr = strchr (buffer, '\n'); > if (end_ptr != NULL) /* chop newline */ > @@ -181,9 +174,40 @@ __getmntent_r (FILE *stream, struct mntent *mp, > char *buffer, int bufsiz) > case 2: > break; > } > + > + return true; > +} > + > +/* Read one mount table entry from STREAM. Returns a pointer to > storage > + reused on the next call, or null for EOF or error (use > feof/ferror to > + check). */ > +struct mntent * > +__getmntent_r (FILE *stream, struct mntent *mp, char *buffer, int > bufsiz) > +{ > + struct mntent *result; > + > + flockfile (stream); > + while (true) > + if (get_mnt_entry (stream, mp, buffer, bufsiz)) > + { > + /* If the file system is autofs look for a mount option hint > + ("ignore") to skip the entry. */ > + if (strcmp (mp->mnt_type, "autofs") == 0 && __hasmntopt (mp, > "ignore")) > + memset (mp, 0, sizeof (*mp)); > + else > + { > + result = mp; > + break; > + } > + } > + else > + { > + result = NULL; > + break; > + } > funlockfile (stream); > > - return mp; > + return result; > } > libc_hidden_def (__getmntent_r) > weak_alias (__getmntent_r, getmntent_r)
On Mon, 2019-09-02 at 13:34 +0200, Florian Weimer wrote: > And here is the new test. I see what you mean about writing the test needing some knowledge of the infrastructure. It's good to have along with the change, thanks. > > Thanks, > Florian > > Add misc/tst-mntent-autofs, testing autofs "ignore" filtering > > 2019-09-02 Florian Weimer <fweimer@redhat.com> > > * misc/tst-mntent-autofs.c: New file. > * misc/Makefile (tests): Add misc/tst-mntent-autofs. > > diff --git a/misc/Makefile b/misc/Makefile > index 032f28fc38..afb8b023e8 100644 > --- a/misc/Makefile > +++ b/misc/Makefile > @@ -86,7 +86,8 @@ tests := tst-dirname tst-tsearch tst-fdset tst- > mntent tst-hsearch \ > tst-mntent-blank-corrupt tst-mntent-blank-passno bug18240 \ > tst-preadvwritev tst-preadvwritev64 tst-makedev tst-empty \ > tst-preadvwritev2 tst-preadvwritev64v2 tst-warn-wide \ > - tst-ldbl-warn tst-ldbl-error tst-dbl-efgcvt tst-ldbl-efgcvt > + tst-ldbl-warn tst-ldbl-error tst-dbl-efgcvt tst-ldbl-efgcvt \ > + tst-mntent-autofs > > # Tests which need libdl. > ifeq (yes,$(build-shared)) > diff --git a/misc/tst-mntent-autofs.c b/misc/tst-mntent-autofs.c > new file mode 100644 > index 0000000000..bf4d4e73b4 > --- /dev/null > +++ b/misc/tst-mntent-autofs.c > @@ -0,0 +1,141 @@ > +/* Test autofs "ignore" filtering for getment_r. > + Copyright (C) 2019 Free Software Foundation, Inc. > + This file is part of the GNU C Library. > + > + The GNU C Library is free software; you can redistribute it > and/or > + modify it under the terms of the GNU Lesser General Public > + License as published by the Free Software Foundation; either > + version 2.1 of the License, or (at your option) any later > version. > + > + The GNU C Library is distributed in the hope that it will be > useful, > + but WITHOUT ANY WARRANTY; without even the implied warranty of > + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU > + Lesser General Public License for more details. > + > + You should have received a copy of the GNU Lesser General Public > + License along with the GNU C Library; if not, see > + <http://www.gnu.org/licenses/>;. */ > + > +#include <array_length.h> > +#include <errno.h> > +#include <mntent.h> > +#include <stdio.h> > +#include <stdlib.h> > +#include <string.h> > +#include <support/check.h> > +#include <support/temp_file.h> > +#include <support/xstdio.h> > +#include <support/xunistd.h> > + > +struct test_case > +{ > + const char *line; > + struct > + { > + /* Like struct mntent, but with const pointers. */ > + const char *mnt_fsname; > + const char *mnt_dir; > + const char *mnt_type; > + const char *mnt_opts; > + int mnt_freq; > + int mnt_passno; > + } expected; > +}; > + > +static struct test_case test_cases[] = > + { > + { "/etc/auto.direct /mnt/auto/1 autofs defaults 0 0", > + { "/etc/auto.direct", "/mnt/auto/1", "autofs", "defaults", 0, > 0 } }, > + > + /* These entries are filtered out. */ > + { "/etc/auto.2 /mnt/auto/2 autofs ignore 0 0", { NULL, } }, > + { "/etc/auto.3 /mnt/auto/3 autofs ignore,other 1 2", { NULL, } > }, > + { "/etc/auto.4 /mnt/auto/4 autofs other,ignore 3 4", { NULL, } > }, > + { "/etc/auto.5 /mnt/auto/5 autofs opt1,ignore,opt2 5 6", { NULL, > } }, > + > + /* Dummy entry to make the desynchronization more obvious. */ > + { "/dev/sda1 / xfs defaults 0 0", > + { "/dev/sda1", "/", "xfs", "defaults", 0, 0 } }, > + > + /* These are not filtered because the file system is not > autofs. */ > + { "/etc/auto.direct /mnt/auto/6 autofs1 ignore 0 0", > + { "/etc/auto.direct", "/mnt/auto/6", "autofs1", "ignore", 0, 0 > } }, > + { "/etc/auto.direct /mnt/auto/7 autofs1 ignore,other 0 0", > + { "/etc/auto.direct", "/mnt/auto/7", "autofs1", > "ignore,other", 0, 0 } }, > + { "/etc/auto.direct /mnt/auto/8 autofs1 other,ignore 0 0", > + { "/etc/auto.direct", "/mnt/auto/8", "autofs1", > "other,ignore", 0, 0 } }, > + { "/etc/auto.direct /mnt/auto/9 autofs1 opt1,ignore,opt2 0 0", > + { "/etc/auto.direct", "/mnt/auto/9", "autofs1", > "opt1,ignore,opt2", } }, > + > + /* These are not filtered because the string "ignore" is not an > + option name. */ > + { "/etc/auto.direct /mnt/auto/10 autofs noignore 1 2", > + { "/etc/auto.direct", "/mnt/auto/10", "autofs", "noignore", 1, > 2 } }, > + { "/etc/auto.direct /mnt/auto/11 autofs noignore,other 0 0", > + { "/etc/auto.direct", "/mnt/auto/11", "autofs", > "noignore,other", } }, > + { "/etc/auto.direct /mnt/auto/12 autofs other,noignore 0 0", > + { "/etc/auto.direct", "/mnt/auto/12", "autofs", > "other,noignore", } }, > + { "/etc/auto.direct /mnt/auto/13 autofs errors=ignore 0 0", > + { "/etc/auto.direct", "/mnt/auto/13", "autofs", > "errors=ignore", } }, > + { "/etc/auto.direct /mnt/auto/14 autofs errors=ignore,other 0 > 0", > + { "/etc/auto.direct", "/mnt/auto/14", "autofs", > + "errors=ignore,other", } }, > + { "/etc/auto.direct /mnt/auto/15 autofs other,errors=ignore 0 > 0", > + { "/etc/auto.direct", "/mnt/auto/15", "autofs", > + "other,errors=ignore", } }, > + > + /* These are not filtered because the string is escaped. '\151' > + is 'i', but it is not actually decoded by the parser. */ > + { "/etc/auto.\\151gnore /mnt/auto/16 autofs \\151gnore 0 0", > + { "/etc/auto.\\151gnore", "/mnt/auto/16", "autofs", > + "\\151gnore", } }, > + }; > + > +static int > +do_test (void) > +{ > + char *path; > + xclose (create_temp_file ("tst-mntent-autofs-", &path)); > + > + /* Write the test file. */ > + FILE *fp = xfopen (path, "w"); > + for (size_t i = 0; i < array_length (test_cases); ++i) > + fprintf (fp, "%s\n", test_cases[i].line); > + xfclose (fp); > + > + /* Open the test file again, this time for parsing. */ > + fp = setmntent (path, "r"); > + TEST_VERIFY_EXIT (fp != NULL); > + char buffer[512]; > + struct mntent me; > + > + for (size_t i = 0; i < array_length (test_cases); ++i) > + { > + if (test_cases[i].expected.mnt_type == NULL) > + continue; > + > + memset (buffer, 0xcc, sizeof (buffer)); > + memset (&me, 0xcc, sizeof (me)); > + struct mntent *pme = getmntent_r (fp, &me, buffer, sizeof > (buffer)); > + TEST_VERIFY_EXIT (pme != NULL); > + TEST_VERIFY (pme == &me); > + TEST_COMPARE_STRING (test_cases[i].expected.mnt_fsname, > me.mnt_fsname); > + TEST_COMPARE_STRING (test_cases[i].expected.mnt_dir, > me.mnt_dir); > + TEST_COMPARE_STRING (test_cases[i].expected.mnt_type, > me.mnt_type); > + TEST_COMPARE_STRING (test_cases[i].expected.mnt_opts, > me.mnt_opts); > + TEST_COMPARE (test_cases[i].expected.mnt_freq, me.mnt_freq); > + TEST_COMPARE (test_cases[i].expected.mnt_passno, > me.mnt_passno); > + } > + > + TEST_VERIFY (getmntent_r (fp, &me, buffer, sizeof (buffer)) == > NULL); > + > + TEST_COMPARE (feof (fp), 1); > + TEST_COMPARE (ferror (fp), 0); > + errno = 0; > + TEST_COMPARE (endmntent (fp), 1); > + TEST_COMPARE (errno, 0); > + free (path); > + return 0; > +} > + > +#include <support/test-driver.c>
* Ian Kent: > On Mon, 2019-09-02 at 13:34 +0200, Florian Weimer wrote: >> * Ian Kent: >> >> > Historically autofs mounts were not included in mount table >> > listings. This is the case in other SysV autofs implementations >> > and was also the case with Linux autofs. >> > >> > But now that /etc/mtab is a symlink to the proc filesystem >> > mount table the autofs mount entries appear in the mount table >> > on Linux. >> >> Overall, this looks good. Thanks. >> >> I added a ChangeLog entry, tweaked the return type of get_mnt_entry >> to >> bool, and avoided the parameter assignment in __getmntent_r. Do you >> think these changes are okay? > > The changes look fine to me, thanks for them and the changelog > entry. Thanks, I've pushed this, along with the new test. Florian
diff --git a/misc/mntent_r.c b/misc/mntent_r.c index 5d88c45c6f..0b180cbb4a 100644 --- a/misc/mntent_r.c +++ b/misc/mntent_r.c @@ -112,26 +112,18 @@ decode_name (char *buf) return buf; } - -/* Read one mount table entry from STREAM. Returns a pointer to storage - reused on the next call, or null for EOF or error (use feof/ferror to - check). */ -struct mntent * -__getmntent_r (FILE *stream, struct mntent *mp, char *buffer, int bufsiz) +static struct mntent * +get_mnt_entry (FILE *stream, struct mntent *mp, char *buffer, int bufsiz) { char *cp; char *head; - flockfile (stream); do { char *end_ptr; if (__fgets_unlocked (buffer, bufsiz, stream) == NULL) - { - funlockfile (stream); return NULL; - } end_ptr = strchr (buffer, '\n'); if (end_ptr != NULL) /* chop newline */ @@ -181,6 +173,36 @@ __getmntent_r (FILE *stream, struct mntent *mp, char *buffer, int bufsiz) case 2: break; } + + return mp; +} + +/* Read one mount table entry from STREAM. Returns a pointer to storage + reused on the next call, or null for EOF or error (use feof/ferror to + check). */ +struct mntent * +__getmntent_r (FILE *stream, struct mntent *mp, char *buffer, int bufsiz) +{ + flockfile (stream); + do + { + mp = get_mnt_entry (stream, mp, buffer, bufsiz); + if (mp) + { + /* If the file system is autofs look for a mount option + hint ("ignore") to skip the entry. */ + if (strcmp (mp->mnt_type, "autofs") == 0) + { + if (__hasmntopt (mp, "ignore")) + { + memset (mp, 0, sizeof(*mp)); + continue; + } + } + } + break; + } + while (1); funlockfile (stream); return mp;
Historically autofs mounts were not included in mount table listings. This is the case in other SysV autofs implementations and was also the case with Linux autofs. But now that /etc/mtab is a symlink to the proc filesystem mount table the autofs mount entries appear in the mount table on Linux. Prior to the symlinking of /etc/mtab mount table it was sufficient to call mount(2) and simply not update /etc/mtab to exclude autofs mounts from mount listings. Also, with the symlinking of /etc/mtab we have seen a shift in usage toward using the proc mount tables directly. But the autofs mount entries need to be retained when coming from the proc file system for applications that need them (largely autofs file system users themselves) so filtering out these entries within the kernel itself can't be done. So it needs be done in user space. There are three reasons to omit the autofs mount entries. One is that certain types of auto-mounts have an autofs mount for every entry in their autofs mount map and these maps can be quite large. This leads to mount table listings containing a lot of unnecessary entries. Also, this change in behaviour between autofs implementations can cause problems for applications that use getmntent(3) in other OS implementations as well as Linux. Lastly, there's very little that user space can do with autofs mount entries since this must be left to the autofs mount owner, typically the automount daemon. But it can also lead to attempts to access automount managed paths resulting mounts being triggered when they aren't needed or mounts staying mounted for much longer thay they need be. While the point of this change ins't to help with these problems (and it can be quite a problem) it may be a welcome side effect. So the Linux autofs file system has been modified to accept a pseudo mount option of "ignore" (as is used in other OS implementations) so that user space can use this as a hint to skip autofs entries on reading the mount table. The Linux autofs automount daemon used getmntent(3) itself and has been modified to use the proc file system directly so that it can "ignore" mount option. The use of this mount option is opt-in and a configuration option has been added which defaults to not use this option so if there are applications that need these entries, other than autofs itself, they can be retained. Also, since this filtering is based on an added mount option earlier versions of Linux autofs iand other autofs file system users will not use the option and so won't be affected by the change. Signed-off-by: Ian Kent <ikent@redhat.com> --- misc/mntent_r.c | 42 ++++++++++++++++++++++++++++++++---------- 1 file changed, 32 insertions(+), 10 deletions(-)