Re: mtab and firmlink loop
Hi Samuel, Quoting Justus Winter (2014-10-02 12:57:52) > Quoting Samuel Thibault (2014-10-02 01:58:11) > > We're having issues with schroot and mtab on exodar. Looking a bit at > > rpctrace, one can see things like this: > > > > /home/srv/chroot/schroot-unpack/youpi/home/srv/chroot/schroot-unpack/youpi/home/srv/chroot/schroot-unpack/youpi/home/srv/chroot/schroot-unpack/youpi/home/srv/chroot/schroot-unpack/youpi/home/srv/chroot/schroot-unpack/youpi/home/srv/chroot/schroot-unpack/youpi/home/srv/chroot/schroot-unpack/youpi/home/schroot/mount/youpi/srv/chroot/schroot-unpack/youpi/servers > > > > I have indeed made a bunch of firmlinks: /srv is a firmlink to > > /home/srv, and /home/srv/chroot/schroot-unpack/youpi/home is a firmlink > > to /home, thus forming a loop. Can mtab properly cope with this? > > No, I'm afraid it doesn't handle this yet :/ Although there is no code to avoid this, I have been unable to reproduce your troubles. Can you please test the patch that I'll send as follow up? Justus
[PATCH] trans/mtab: avoid firmlink loops
* trans/mtab.c (struct mtab): Add a hash table to keep track of seen
ports.
(mtab_mark_as_seen): New function that records the identity port of a
given node in the hash table and reports whether it has been there
before.
(mtab_populate): Use the new function to avoid running in circles.
(main, open_hook): Initialize hash table.
(close_hook): Free ports and destroy hash table.
---
trans/mtab.c | 46 +-
1 file changed, 45 insertions(+), 1 deletion(-)
diff --git a/trans/mtab.c b/trans/mtab.c
index 5207c1e..a9928b3 100644
--- a/trans/mtab.c
+++ b/trans/mtab.c
@@ -24,6 +24,7 @@
#include
#include
#include
+#include
#include
#include
#include
@@ -55,6 +56,7 @@ struct mtab
char *contents;
size_t contents_len;
off_t offs;
+ struct hurd_ihash ports_seen;
};
const char *argp_program_version = STANDARD_HURD_VERSION (mtab);
@@ -244,7 +246,11 @@ main (int argc, char *argv[])
else
{
/* One-shot mode. */
- struct mtab mtab = { .lock = PTHREAD_MUTEX_INITIALIZER };
+ struct mtab mtab =
+{
+ .lock = PTHREAD_MUTEX_INITIALIZER,
+ .ports_seen = HURD_IHASH_INITIALIZER (HURD_IHASH_NO_LOCP),
+};
err = mtab_populate (&mtab, target_path, insecure);
if (err)
error (5, err, "%s", target_path);
@@ -301,6 +307,33 @@ is_filesystem_translator (file_t node)
}
}
+/* Records NODE's idport in ports_seen, returns true if we have
+ already seen this node or there was an error getting the id
+ port. */
+boolean_t
+mtab_mark_as_seen (struct mtab *mtab, mach_port_t node)
+{
+ error_t err;
+ mach_port_t idport, fsidport;
+ ino_t fileno;
+
+ err = io_identity (node, &idport, &fsidport, &fileno);
+ if (err)
+return TRUE;
+
+ mach_port_deallocate (mach_task_self (), fsidport);
+
+ if (hurd_ihash_find (&mtab->ports_seen, idport))
+{
+ /* Already seen. Get rid of the extra reference. */
+ mach_port_deallocate (mach_task_self (), idport);
+ return TRUE;
+}
+
+ hurd_ihash_add (&mtab->ports_seen, idport, idport);
+ return FALSE;
+}
+
/* Populates the given MTAB object with the information for PATH. If
INSECURE is given, also follow translators bound to nodes not owned
by root or the current user. */
@@ -363,6 +396,13 @@ mtab_populate (struct mtab *mtab, const char *path, int
insecure)
goto errout;
}
+ /* Avoid running in circles. */
+ if (mtab_mark_as_seen (mtab, node))
+{
+ err = 0;
+ goto errout;
+}
+
/* Query its options. */
err = file_get_fs_options (node, &argz, &argz_len);
if (err)
@@ -602,6 +642,7 @@ open_hook (struct trivfs_peropen *peropen)
mtab->offs = 0;
mtab->contents = NULL;
mtab->contents_len = 0;
+ hurd_ihash_init (&mtab->ports_seen, HURD_IHASH_NO_LOCP);
/* The mtab object is initialized, but not yet populated. We delay
that until that data is really needed. This avoids the following
@@ -635,6 +676,9 @@ close_hook (struct trivfs_peropen *peropen)
struct mtab *op = peropen->hook;
pthread_mutex_destroy (&op->lock);
free (op->contents);
+ HURD_IHASH_ITERATE (&op->ports_seen, p)
+mach_port_deallocate (mach_task_self (), (mach_port_t) p);
+ hurd_ihash_destroy (&op->ports_seen);
free (op);
}
--
2.1.1
Re: [PATCH 2/2] libports: lock-less reference counting for port_info objects
Ack, thanks!
Justus Winter, le Fri 05 Sep 2014 17:57:22 +0200, a écrit :
> * libports/ports.h (struct port_info): Use the new type.
> * libports/lookup-port.c: No need to lock _ports_lock anymore.
> * libports/bucket-iterate.c: Likewise.
> * libports/complete-deallocate.c: Check if someone reacquired a
> reference through a hash table lookup.
> * libports/create-internal.c: Use the new reference counting primitives.
> * libports/get-right.c: Likewise.
> * libports/import-port.c: Likewise.
> * libports/port-deref-weak.c: Likewise.
> * libports/port-deref.c: Likewise.
> * libports/port-ref-weak.c: Likewise.
> * libports/port-ref.c: Likewise.
> * libports/reallocate-from-external.c: Likewise.
> * libports/transfer-right.c: Likewise.
> * utils/rpctrace.c: Likewise.
> ---
> libports/bucket-iterate.c | 4 +---
> libports/complete-deallocate.c | 14 ++
> libports/create-internal.c | 3 +--
> libports/get-right.c| 2 +-
> libports/import-port.c | 3 +--
> libports/lookup-port.c | 4 +---
> libports/port-deref-weak.c | 10 +++---
> libports/port-deref.c | 34 --
> libports/port-ref-weak.c| 6 +-
> libports/port-ref.c | 6 +-
> libports/ports.h| 4 ++--
> libports/reallocate-from-external.c | 2 +-
> libports/transfer-right.c | 2 +-
> utils/rpctrace.c| 10 --
> 14 files changed, 52 insertions(+), 52 deletions(-)
>
> diff --git a/libports/bucket-iterate.c b/libports/bucket-iterate.c
> index 79b6d72..b021b99 100644
> --- a/libports/bucket-iterate.c
> +++ b/libports/bucket-iterate.c
> @@ -35,7 +35,6 @@ _ports_bucket_class_iterate (struct hurd_ihash *ht,
>size_t i, n, nr_items;
>error_t err;
>
> - pthread_mutex_lock (&_ports_lock);
>pthread_rwlock_rdlock (&_ports_htable_lock);
>
>if (ht->nr_items == 0)
> @@ -59,13 +58,12 @@ _ports_bucket_class_iterate (struct hurd_ihash *ht,
>
>if (class == 0 || pi->class == class)
> {
> - pi->refcnt++;
> + refcounts_ref (&pi->refcounts, NULL);
> p[n] = pi;
> n++;
> }
> }
>pthread_rwlock_unlock (&_ports_htable_lock);
> - pthread_mutex_unlock (&_ports_lock);
>
>if (n != 0 && n != nr_items)
> {
> diff --git a/libports/complete-deallocate.c b/libports/complete-deallocate.c
> index 4768dab..0d852f5 100644
> --- a/libports/complete-deallocate.c
> +++ b/libports/complete-deallocate.c
> @@ -29,15 +29,29 @@ _ports_complete_deallocate (struct port_info *pi)
>
>if (pi->port_right)
> {
> + struct references result;
> +
>pthread_rwlock_wrlock (&_ports_htable_lock);
> + refcounts_references (&pi->refcounts, &result);
> + if (result.hard > 0 || result.weak > 0)
> +{
> + /* A reference was reacquired through a hash table lookup.
> + It's fine, we didn't touch anything yet. */
> + pthread_mutex_unlock (&_ports_htable_lock);
> + return;
> +}
> +
>hurd_ihash_locp_remove (&_ports_htable, pi->ports_htable_entry);
>hurd_ihash_locp_remove (&pi->bucket->htable, pi->hentry);
>pthread_rwlock_unlock (&_ports_htable_lock);
> +
>mach_port_mod_refs (mach_task_self (), pi->port_right,
> MACH_PORT_RIGHT_RECEIVE, -1);
>pi->port_right = MACH_PORT_NULL;
> }
>
> + pthread_mutex_lock (&_ports_lock);
> +
>pi->bucket->count--;
>pi->class->count--;
>
> diff --git a/libports/create-internal.c b/libports/create-internal.c
> index 8543986..2d85931 100644
> --- a/libports/create-internal.c
> +++ b/libports/create-internal.c
> @@ -54,8 +54,7 @@ _ports_create_port_internal (struct port_class *class,
> }
>
>pi->class = class;
> - pi->refcnt = 1;
> - pi->weakrefcnt = 0;
> + refcounts_init (&pi->refcounts, 1, 0);
>pi->cancel_threshold = 0;
>pi->mscount = 0;
>pi->flags = 0;
> diff --git a/libports/get-right.c b/libports/get-right.c
> index 89050c6..8681f46 100644
> --- a/libports/get-right.c
> +++ b/libports/get-right.c
> @@ -41,7 +41,7 @@ ports_get_right (void *port)
>if ((pi->flags & PORT_HAS_SENDRIGHTS) == 0)
> {
>pi->flags |= PORT_HAS_SENDRIGHTS;
> - pi->refcnt++;
> + refcounts_ref (&pi->refcounts, NULL);
>err = mach_port_request_notification (mach_task_self (),
> pi->port_right,
> MACH_NOTIFY_NO_SENDERS,
> diff --git a/libports/import-port.c b/libports/import-port.c
> index 2660672..c337c85 100644
> --- a/libports/import-port.c
> +++ b/libports/import-port.c
> @@ -48,8 +48,7 @@ ports_import_port (struct port_class *class, struct
> port_bucket *bucket,
> return ENOMEM;
>
>pi->class = class;
> - pi->refcnt = 1 + !!stat.mps_srights;
> - pi->weakrefcnt = 0;
> + refcounts_init
Re: [PATCH v2] utils/mount: add mount options to create firmlinks.
Gabriele Giacone, le Tue 27 May 2014 22:20:29 +0200, a écrit :
> * utils/mount.c (parse_opt): Add -B/--bind/--firmlink/-o bind mount
> options. (do_mount): Do not pass bind mount option to settrans, set
> firmlink fstype. (main): Likewise.
Applied, thanks!
> ---
> utils/mount.c | 39 ---
> 1 file changed, 32 insertions(+), 7 deletions(-)
>
> diff --git a/utils/mount.c b/utils/mount.c
> index df77c66..c5736ba 100644
> --- a/utils/mount.c
> +++ b/utils/mount.c
> @@ -64,6 +64,8 @@ static const struct argp_option argp_opts[] =
>{"no-mtab", 'n', 0, 0, "Do not update /etc/mtab"},
>{"test-opts", 'O', "OPTIONS", 0,
> "Only mount fstab entries matching the given set of options"},
> + {"bind", 'B', 0, 0, "Bind mount, firmlink"},
> + {"firmlink", 0, 0, OPTION_ALIAS},
>{"fake", 'f', 0, 0, "Do not actually mount, just pretend"},
>{0, 0}
> };
> @@ -87,6 +89,7 @@ parse_opt (int key, char *arg, struct argp_state *state)
> case 'r': ARGZ (add (&options, &options_len, "ro"));
> case 'w': ARGZ (add (&options, &options_len, "rw"));
> case 'u': ARGZ (add (&options, &options_len, "update"));
> +case 'B': ARGZ (add (&options, &options_len, "bind"));
> case 'o': ARGZ (add_sep (&options, &options_len, arg, ','));
> case 'v': ++verbose; break;
> #undef ARGZ
> @@ -250,12 +253,20 @@ do_mount (struct fs *fs, int remount)
>/* Append the fstab options to any specified on the command line. */
>ARGZ (create_sep (fs->mntent.mnt_opts, ',', &mntopts, &mntopts_len));
>
> - /* Remove the `noauto' option, since it's for us not the filesystem.
> */
> + /* Remove the `noauto' and `bind' options, since they're for us not the
> + filesystem. */
>for (o = mntopts; o; o = argz_next (mntopts, mntopts_len, o))
> - if (!strcmp (o, MNTOPT_NOAUTO))
> - break;
> - if (o)
> - argz_delete (&mntopts, &mntopts_len, o);
> +{
> + if (strcmp (o, MNTOPT_NOAUTO) == 0)
> +argz_delete (&mntopts, &mntopts_len, o);
> + if (strcmp (o, "bind") == 0)
> +{
> + fs->mntent.mnt_type = strdup ("firmlink");
> + if (!fs->mntent.mnt_type)
> +error (3, ENOMEM, "failed to allocate memory");
> + argz_delete (&mntopts, &mntopts_len, o);
> +}
> +}
>
>ARGZ (append (&mntopts, &mntopts_len, options, options_len));
> }
> @@ -273,7 +284,7 @@ do_mount (struct fs *fs, int remount)
>{
> ARGZ (add (&fsopts, &fsopts_len, o));
>}
> -else if (strcmp (o, "defaults") != 0)
> +else if ((strcmp (o, "defaults") != 0) && (strlen (o) != 0))
>{
> /* Prepend `--' to the option to make a long option switch,
> e.g. `--ro' or `--rsize=1024'. */
> @@ -572,7 +583,7 @@ do_query (struct fs *fs)
> int
> main (int argc, char **argv)
> {
> - unsigned int remount;
> + unsigned int remount, firmlink;
>struct fstab *fstab;
>struct fs *fs;
>error_t err;
> @@ -598,6 +609,16 @@ main (int argc, char **argv)
>if (err)
> error (3, ENOMEM, "collecting mount options");
>
> + /* Do not pass `bind' option to firmlink translator */
> + char *opt = NULL;
> + firmlink = 0;
> + while ((opt = argz_next (options, options_len, opt)))
> +if (strcmp (opt, "bind") == 0)
> + {
> +firmlink = 1;
> +argz_delete(&options, &options_len, opt);
> + }
> +
>if (device)/* two-argument form */
> {
>struct mntent m =
> @@ -608,6 +629,8 @@ main (int argc, char **argv)
> mnt_opts: 0,
> mnt_freq: 0, mnt_passno: 0
>};
> + if (firmlink)
> +m.mnt_type = strdup ("firmlink");
>
>err = fstab_add_mntent (fstab, &m, &fs);
>if (err)
> @@ -625,6 +648,8 @@ main (int argc, char **argv)
> mnt_opts: 0,
> mnt_freq: 0, mnt_passno: 0
>};
> + if (firmlink)
> +m.mnt_type = strdup ("firmlink");
>
>err = fstab_add_mntent (fstab, &m, &fs);
>if (err)
> --
> 2.0.0.rc4
>
--
Samuel
Tu as lu les docs. Tu es devenu un informaticien. Que tu le veuilles
ou non. Lire la doc, c'est le Premier et Unique Commandement de
l'informaticien.
-+- TP in: Guide du Linuxien pervers - "L'évangile selon St Thomas"
Re: [PATCH 2/8] procfs: do not hard-code the default argument values
Justus Winter, le Fri 05 Sep 2014 10:42:56 +0200, a écrit :
> * procfs/main.c (common_options): If possible, do not hard-code the
> default values.
Ack.
> ---
> procfs/main.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/procfs/main.c b/procfs/main.c
> index f3067d9..36a2d25 100644
> --- a/procfs/main.c
> +++ b/procfs/main.c
> @@ -142,13 +142,13 @@ struct argp_option common_options[] = {
> #define XSTR(X) #X
>{ "clk-tck", 'h', "HZ", 0,
>"Unit used for the values expressed in system clock ticks "
> - "(default: sysconf(_SC_CLK_TCK))" },
> + "(default: " STR (OPT_CLK_TCK) ")" },
>{ "stat-mode", 's', "MODE", 0,
>"The [pid]/stat file publishes information which on Hurd is only "
>"available to the process owner. "
>"You can use this option to override its mode to be more permissive "
>"for compatibility purposes. "
> - "(default: 0400)" },
> + "(default: " STR (OPT_STAT_MODE) ")" },
>{ "fake-self", 'S', "PID", OPTION_ARG_OPTIONAL,
>"Provide a fake \"self\" symlink to the given PID, for compatibility "
>"purposes. If PID is omitted, \"self\" will point to init. "
> @@ -164,7 +164,7 @@ struct argp_option common_options[] = {
>"Make USER the owner of files related to processes without one. "
>"Be aware that USER will be granted access to the environment and "
>"other sensitive information about the processes in question. "
> - "(default: use uid 0)" },
> + "(default: use uid " STR (OPT_ANON_OWNER) ")" },
>{ "nodev", NODEV_KEY, NULL, 0,
>"Ignored for compatibility with Linux' procfs." },
>{ "noexec", NOEXEC_KEY, NULL, 0,
> --
> 2.1.0
>
--
Samuel
"...Unix, MS-DOS, and Windows NT (also known as the Good, the Bad, and
the Ugly)."
(By Matt Welsh)
