Looks nice. Minor nitpicks below. On Sun, Mar 22, 2015 at 1:36 PM, Michael Olbrich <m.olbr...@pengutronix.de> wrote: > --- > man/systemd.automount.xml | 8 ++ > man/systemd.mount.xml | 9 ++ > src/core/automount.c | 209 > ++++++++++++++++++++++++++++++++-- > src/core/automount.h | 6 +- > src/core/dbus-automount.c | 1 + > src/core/load-fragment-gperf.gperf.m4 | 1 + > src/core/mount.c | 20 +--- > src/fstab-generator/fstab-generator.c | 27 +++++ > 8 files changed, 253 insertions(+), 28 deletions(-) > > diff --git a/man/systemd.automount.xml b/man/systemd.automount.xml > index b5b5885cdff2..9561590c5c89 100644 > --- a/man/systemd.automount.xml > +++ b/man/systemd.automount.xml > @@ -135,6 +135,14 @@ > creating these directories. Takes an access mode in octal > notation. Defaults to 0755.</para></listitem> > </varlistentry> > + <varlistentry> > + <term><varname>TimeoutIdleSec=</varname></term> > + <listitem><para>Configures an idleness timeout. Once the mount has > been > + idle for the specified time, systemd will attempt to unmount. Takes a > + unit-less value in seconds, or a time span value such as "5min 20s". > + Pass 0 to disable the timeout logic. The timeout is disabled by > + default.</para></listitem> > + </varlistentry> > </variablelist> > </refsect1> > > diff --git a/man/systemd.mount.xml b/man/systemd.mount.xml > index fcb9a4416104..e102d27ab762 100644 > --- a/man/systemd.mount.xml > +++ b/man/systemd.mount.xml > @@ -148,6 +148,15 @@ > </varlistentry> > > <varlistentry> > + <term><option>x-systemd.idle-timeout=</option></term> > + > + <listitem><para>Configures the idleness timeout of the > + automount unit. See <varname>TimeoutIdleSec=</varname> in > + > <citerefentry><refentrytitle>systemd.automount</refentrytitle><manvolnum>5</manvolnum></citerefentry> > + for details.</para></listitem> > + </varlistentry> > + > + <varlistentry> > <term><option>x-systemd.device-timeout=</option></term> > > <listitem><para>Configure how long systemd should wait for a > diff --git a/src/core/automount.c b/src/core/automount.c > index cec90cbb319d..d2c148943e0f 100644 > --- a/src/core/automount.c > +++ b/src/core/automount.c > @@ -40,6 +40,7 @@ > #include "dbus-automount.h" > #include "bus-util.h" > #include "bus-error.h" > +#include "async.h" > > static const UnitActiveState state_translation_table[_AUTOMOUNT_STATE_MAX] = > { > [AUTOMOUNT_DEAD] = UNIT_INACTIVE, > @@ -79,13 +80,16 @@ static void repeat_unmount(const char *path) { > } > } > > +static int automount_send_ready(Automount *a, Set *tokens, int status); > + > static void unmount_autofs(Automount *a) { > assert(a); > > if (a->pipe_fd < 0) > return; > > - automount_send_ready(a, -EHOSTDOWN); > + automount_send_ready(a, a->tokens, -EHOSTDOWN); > + automount_send_ready(a, a->expire_tokens, -EHOSTDOWN); > > a->pipe_event_source = sd_event_source_unref(a->pipe_event_source); > a->pipe_fd = safe_close(a->pipe_fd); > @@ -110,6 +114,8 @@ static void automount_done(Unit *u) { > > set_free(a->tokens); > a->tokens = NULL; > + set_free(a->expire_tokens); > + a->expire_tokens = NULL; > } > > static int automount_add_mount_links(Automount *a) { > @@ -263,6 +269,7 @@ static int automount_coldplug(Unit *u, Hashmap > *deferred_work) { > } > > static void automount_dump(Unit *u, FILE *f, const char *prefix) { > + char time_string[FORMAT_TIMESPAN_MAX]; > Automount *a = AUTOMOUNT(u); > > assert(a); > @@ -271,11 +278,13 @@ static void automount_dump(Unit *u, FILE *f, const char > *prefix) { > "%sAutomount State: %s\n" > "%sResult: %s\n" > "%sWhere: %s\n" > - "%sDirectoryMode: %04o\n", > + "%sDirectoryMode: %04o\n" > + "%sTimeoutIdleUSec: %s\n", > prefix, automount_state_to_string(a->state), > prefix, automount_result_to_string(a->result), > prefix, a->where, > - prefix, a->directory_mode); > + prefix, a->directory_mode, > + prefix, format_timespan(time_string, FORMAT_TIMESPAN_MAX, > a->timeout_idle_usec, USEC_PER_SEC)); > } > > static void automount_enter_dead(Automount *a, AutomountResult f) { > @@ -365,7 +374,7 @@ static int autofs_protocol(int dev_autofs_fd, int > ioctl_fd) { > return 0; > } > > -static int autofs_set_timeout(int dev_autofs_fd, int ioctl_fd, time_t sec) { > +static int autofs_set_timeout(int dev_autofs_fd, int ioctl_fd, usec_t usec) { > struct autofs_dev_ioctl param; > > assert(dev_autofs_fd >= 0); > @@ -373,7 +382,9 @@ static int autofs_set_timeout(int dev_autofs_fd, int > ioctl_fd, time_t sec) { > > init_autofs_dev_ioctl(¶m); > param.ioctlfd = ioctl_fd; > - param.timeout.timeout = sec; > + > + /* Convert to seconds, rounding up. */ > + param.timeout.timeout = usec / USEC_PER_SEC + (usec % USEC_PER_SEC > > 0);
This is an odd way to round, no? Usually we do (usec + USEC_PER_SEC - 1) / USEC_PER_SEC; > if (ioctl(dev_autofs_fd, AUTOFS_DEV_IOCTL_TIMEOUT, ¶m) < 0) > return -errno; > @@ -402,7 +413,7 @@ static int autofs_send_ready(int dev_autofs_fd, int > ioctl_fd, uint32_t token, in > return 0; > } > > -int automount_send_ready(Automount *a, int status) { > +static int automount_send_ready(Automount *a, Set *tokens, int status) { > _cleanup_close_ int ioctl_fd = -1; > unsigned token; > int r; > @@ -410,7 +421,7 @@ int automount_send_ready(Automount *a, int status) { > assert(a); > assert(status <= 0); > > - if (set_isempty(a->tokens)) > + if (set_isempty(tokens)) > return 0; > > ioctl_fd = open_ioctl_fd(UNIT(a)->manager->dev_autofs_fd, a->where, > a->dev_id); > @@ -425,7 +436,7 @@ int automount_send_ready(Automount *a, int status) { > r = 0; > > /* Autofs thankfully does not hand out 0 as a token */ > - while ((token = PTR_TO_UINT(set_steal_first(a->tokens)))) { > + while ((token = PTR_TO_UINT(set_steal_first(tokens)))) { > int k; > > /* Autofs fun fact II: > @@ -444,6 +455,45 @@ int automount_send_ready(Automount *a, int status) { > return r; > } > > +int automount_update_mount(Automount *a, MountState old_state, MountState > state) > +{ > + _cleanup_close_ int ioctl_fd = -1; > + > + assert(a); These should be switch() statements, I guess: > + if (state == MOUNT_MOUNTED || state == MOUNT_REMOUNTING) > + automount_send_ready(a, a->tokens, 0); > + else if (state == MOUNT_DEAD || > + state == MOUNT_UNMOUNTING || > + state == MOUNT_MOUNTING_SIGTERM || > + state == MOUNT_MOUNTING_SIGKILL || > + state == MOUNT_REMOUNTING_SIGTERM || > + state == MOUNT_REMOUNTING_SIGKILL || > + state == MOUNT_UNMOUNTING_SIGTERM || > + state == MOUNT_UNMOUNTING_SIGKILL || > + state == MOUNT_FAILED) { > + if (old_state != state) > + automount_send_ready(a, a->tokens, -ENODEV); > + } > + > + if (state == MOUNT_DEAD) > + automount_send_ready(a, a->expire_tokens, 0); > + else if (state == MOUNT_MOUNTING || > + state == MOUNT_MOUNTING_DONE || > + state == MOUNT_MOUNTING_SIGTERM || > + state == MOUNT_MOUNTING_SIGKILL || > + state == MOUNT_REMOUNTING_SIGTERM || > + state == MOUNT_REMOUNTING_SIGKILL || > + state == MOUNT_UNMOUNTING_SIGTERM || > + state == MOUNT_UNMOUNTING_SIGKILL || > + state == MOUNT_FAILED) { > + if (old_state != state) > + automount_send_ready(a, a->expire_tokens, -ENODEV); > + } > + > + return 0; > +} > + > static void automount_enter_waiting(Automount *a) { > _cleanup_close_ int ioctl_fd = -1; > int p[2] = { -1, -1 }; > @@ -503,7 +553,7 @@ static void automount_enter_waiting(Automount *a) { > if (r < 0) > goto fail; > > - r = autofs_set_timeout(dev_autofs_fd, ioctl_fd, 300); > + r = autofs_set_timeout(dev_autofs_fd, ioctl_fd, > a->timeout_idle_usec); > if (r < 0) > goto fail; > > @@ -535,6 +585,100 @@ fail: > automount_enter_dead(a, AUTOMOUNT_FAILURE_RESOURCES); > } > > +static int automount_start_expire(Automount *a); > > +struct expire_data { > + int dev_autofs_fd; > + int ioctl_fd; > +}; Structs at the top of the file please. > +static inline void cleanup_expire(void *p) { > + struct expire_data *data = *(struct expire_data**)p; > + > + if (!data) > + return; > + > + safe_close(data->dev_autofs_fd); > + safe_close(data->ioctl_fd); > + free(data); > +} > +#define _cleanup_expire_ _cleanup_(cleanup_expire) No need to #define this for internal stuff that is only used a few times. However, you should use DEFINE_TRIVIAL_CLEANUP_FUNC() to get the types right, then just use _cleanup_(cleanup_expirep) in your code (note the 'p'). > +static void *expire_thread(void *p) { > + struct autofs_dev_ioctl param; > + _cleanup_expire_ struct expire_data *data = (struct expire_data*)p; > + int r; > + > + assert(data->dev_autofs_fd >= 0); > + assert(data->ioctl_fd >= 0); > + > + init_autofs_dev_ioctl(¶m); > + param.ioctlfd = data->ioctl_fd; > + > + do { > + r = ioctl(data->dev_autofs_fd, AUTOFS_DEV_IOCTL_EXPIRE, > ¶m); > + } while (r == 0); Could this be "r >= 0" instead, to make it clearer that the next condition is only hit when r < 0 (so errno is guaranteed to be set correctly). > + if (errno != EAGAIN) > + log_error_errno(errno, "failed to expire automount: %m"); No need for this, it is equivalent to log_error("failed to exprie automount: %m"); > + return NULL; > +} > + > +static int automount_dispatch_expire(sd_event_source *source, usec_t usec, > void *userdata) { > + Automount *a = AUTOMOUNT(userdata); > + _cleanup_expire_ struct expire_data *data = NULL; > + int r; > + > + assert(a); > + assert(source == a->expire_event_source); > + > + data = malloc0(sizeof(struct expire_data)); > + if (!data) > + return -ENOMEM; > + > + data->ioctl_fd = -1; > + > + data->dev_autofs_fd = dup(UNIT(a)->manager->dev_autofs_fd); > + if (data->dev_autofs_fd < 0) > + return data->dev_autofs_fd; > + > + data->ioctl_fd = open_ioctl_fd(UNIT(a)->manager->dev_autofs_fd, > a->where, a->dev_id); > + if (data->ioctl_fd < 0) > + return data->ioctl_fd; > + > + r = asynchronous_job(expire_thread, data); > + if (r < 0) { > + log_unit_error(UNIT(a)->id, "Failed to start expire job: > %s", strerror(-r)); > + return r; > + } > + > + data = NULL; > + > + return automount_start_expire(a); > +} > + > +static int automount_start_expire(Automount *a) { > + int r; > + > + assert(a); > + > + if (a->expire_event_source) { > + r = sd_event_source_set_time(a->expire_event_source, > now(CLOCK_MONOTONIC) + 5 * USEC_PER_SEC); > + if (r < 0) > + return r; > + > + return sd_event_source_set_enabled(a->expire_event_source, > SD_EVENT_ONESHOT); > + } > + > + return sd_event_add_time( > + UNIT(a)->manager->event, > + &a->expire_event_source, > + CLOCK_MONOTONIC, > + now(CLOCK_MONOTONIC) + 5 * USEC_PER_SEC, 0, > + automount_dispatch_expire, a); > +} > + > static void automount_enter_runnning(Automount *a) { > _cleanup_bus_error_free_ sd_bus_error error = SD_BUS_ERROR_NULL; > struct stat st; > @@ -547,7 +691,8 @@ static void automount_enter_runnning(Automount *a) { > if (unit_stop_pending(UNIT(a))) { > log_unit_debug(UNIT(a)->id, > "Suppressing automount request on %s since > unit stop is scheduled.", UNIT(a)->id); > - automount_send_ready(a, -EHOSTDOWN); > + automount_send_ready(a, a->tokens, -EHOSTDOWN); > + automount_send_ready(a, a->expire_tokens, -EHOSTDOWN); > return; > } > > @@ -573,6 +718,7 @@ static void automount_enter_runnning(Automount *a) { > goto fail; > } > } > + automount_start_expire(a); > > automount_set_state(a, AUTOMOUNT_RUNNING); > return; > @@ -627,6 +773,8 @@ static int automount_serialize(Unit *u, FILE *f, FDSet > *fds) { > > SET_FOREACH(p, a->tokens, i) > unit_serialize_item_format(u, f, "token", "%u", > PTR_TO_UINT(p)); > + SET_FOREACH(p, a->expire_tokens, i) > + unit_serialize_item_format(u, f, "expire-token", "%u", > PTR_TO_UINT(p)); > > if (a->pipe_fd >= 0) { > int copy; > @@ -686,6 +834,20 @@ static int automount_deserialize_item(Unit *u, const > char *key, const char *valu > if (r < 0) > return r; > } > + } else if (streq(key, "expire-token")) { > + unsigned token; > + > + if (safe_atou(value, &token) < 0) > + log_unit_debug(u->id, "Failed to parse token value > %s", value); > + else { > + if (!a->expire_tokens) > + if (!(a->expire_tokens = set_new(NULL))) > + return -ENOMEM; > + > + r = set_put(a->expire_tokens, UINT_TO_PTR(token)); > + if (r < 0) > + return r; > + } > } else if (streq(key, "pipe-fd")) { > int fd; > > @@ -723,6 +885,7 @@ static bool automount_check_gc(Unit *u) { > } > > static int automount_dispatch_io(sd_event_source *s, int fd, uint32_t > events, void *userdata) { > + _cleanup_bus_error_free_ sd_bus_error error = SD_BUS_ERROR_NULL; > union autofs_v5_packet_union packet; > Automount *a = AUTOMOUNT(userdata); > int r; > @@ -770,6 +933,32 @@ static int automount_dispatch_io(sd_event_source *s, int > fd, uint32_t events, vo > automount_enter_runnning(a); > break; > > + case autofs_ptype_expire_direct: > + log_unit_debug(UNIT(a)->id, "Got direct umount request on > %s", a->where); > + > + sd_event_source_set_enabled(a->expire_event_source, > SD_EVENT_OFF); > + > + r = set_ensure_allocated(&a->expire_tokens, NULL); > + if (r < 0) { > + log_unit_error(UNIT(a)->id, "Failed to allocate > token set."); > + goto fail; > + } > + > + r = set_put(a->expire_tokens, > UINT_TO_PTR(packet.v5_packet.wait_queue_token)); > + if (r < 0) { > + log_unit_error_errno(UNIT(a)->id, r, "Failed to > remember token: %m"); > + goto fail; > + } > + r = manager_add_job(UNIT(a)->manager, JOB_STOP, > UNIT_TRIGGER(UNIT(a)), > + JOB_REPLACE, true, &error, NULL); > + if (r < 0) { > + log_unit_warning(UNIT(a)->id, > + "%s failed to queue umount startup > job: %s", > + UNIT(a)->id, > bus_error_message(&error, r)); > + goto fail; > + } > + break; > + > default: > log_unit_error(UNIT(a)->id, "Received unknown automount > request %i", packet.hdr.type); > break; > diff --git a/src/core/automount.h b/src/core/automount.h > index 60f55223897e..2a50fef68d01 100644 > --- a/src/core/automount.h > +++ b/src/core/automount.h > @@ -47,6 +47,7 @@ struct Automount { > AutomountState state, deserialized_state; > > char *where; > + usec_t timeout_idle_usec; > > int pipe_fd; > sd_event_source *pipe_event_source; > @@ -54,13 +55,16 @@ struct Automount { > dev_t dev_id; > > Set *tokens; > + Set *expire_tokens; > + > + sd_event_source *expire_event_source; > > AutomountResult result; > }; > > extern const UnitVTable automount_vtable; > > -int automount_send_ready(Automount *a, int status); > +int automount_update_mount(Automount *a, MountState old_state, MountState > state); > > const char* automount_state_to_string(AutomountState i) _const_; > AutomountState automount_state_from_string(const char *s) _pure_; > diff --git a/src/core/dbus-automount.c b/src/core/dbus-automount.c > index 38acbd0c2625..5162ce34cb75 100644 > --- a/src/core/dbus-automount.c > +++ b/src/core/dbus-automount.c > @@ -30,5 +30,6 @@ const sd_bus_vtable bus_automount_vtable[] = { > SD_BUS_PROPERTY("Where", "s", NULL, offsetof(Automount, where), > SD_BUS_VTABLE_PROPERTY_CONST), > SD_BUS_PROPERTY("DirectoryMode", "u", bus_property_get_mode, > offsetof(Automount, directory_mode), SD_BUS_VTABLE_PROPERTY_CONST), > SD_BUS_PROPERTY("Result", "s", property_get_result, > offsetof(Automount, result), SD_BUS_VTABLE_PROPERTY_EMITS_CHANGE), > + SD_BUS_PROPERTY("TimeoutIdleUSec", "t", bus_property_get_usec, > offsetof(Automount, timeout_idle_usec), SD_BUS_VTABLE_PROPERTY_CONST), > SD_BUS_VTABLE_END > }; > diff --git a/src/core/load-fragment-gperf.gperf.m4 > b/src/core/load-fragment-gperf.gperf.m4 > index 53059845e30c..66c9145aa6ab 100644 > --- a/src/core/load-fragment-gperf.gperf.m4 > +++ b/src/core/load-fragment-gperf.gperf.m4 > @@ -318,6 +318,7 @@ KILL_CONTEXT_CONFIG_ITEMS(Mount)m4_dnl > m4_dnl > Automount.Where, config_parse_path, 0, > offsetof(Automount, where) > Automount.DirectoryMode, config_parse_mode, 0, > offsetof(Automount, directory_mode) > +Automount.TimeoutIdleSec, config_parse_sec, 0, > offsetof(Automount, timeout_idle_usec) > m4_dnl > Swap.What, config_parse_path, 0, > offsetof(Swap, parameters_fragment.what) > Swap.Priority, config_parse_int, 0, > offsetof(Swap, parameters_fragment.priority) > diff --git a/src/core/mount.c b/src/core/mount.c > index 1251c94608d6..4154dd708959 100644 > --- a/src/core/mount.c > +++ b/src/core/mount.c > @@ -545,7 +545,7 @@ static int mount_load(Unit *u) { > return mount_verify(m); > } > > -static int mount_notify_automount(Mount *m, int status) { > +static int mount_notify_automount(Mount *m, MountState old_state, MountState > state) { > Unit *p; > int r; > Iterator i; > @@ -554,7 +554,7 @@ static int mount_notify_automount(Mount *m, int status) { > > SET_FOREACH(p, UNIT(m)->dependencies[UNIT_TRIGGERED_BY], i) > if (p->type == UNIT_AUTOMOUNT) { > - r = automount_send_ready(AUTOMOUNT(p), status); > + r = automount_update_mount(AUTOMOUNT(p), old_state, > state); > if (r < 0) > return r; > } > @@ -585,21 +585,7 @@ static void mount_set_state(Mount *m, MountState state) { > m->control_command_id = _MOUNT_EXEC_COMMAND_INVALID; > } > > - if (state == MOUNT_MOUNTED || > - state == MOUNT_REMOUNTING) > - mount_notify_automount(m, 0); > - else if (state == MOUNT_DEAD || > - state == MOUNT_UNMOUNTING || > - state == MOUNT_MOUNTING_SIGTERM || > - state == MOUNT_MOUNTING_SIGKILL || > - state == MOUNT_REMOUNTING_SIGTERM || > - state == MOUNT_REMOUNTING_SIGKILL || > - state == MOUNT_UNMOUNTING_SIGTERM || > - state == MOUNT_UNMOUNTING_SIGKILL || > - state == MOUNT_FAILED) { > - if (state != old_state) > - mount_notify_automount(m, -ENODEV); > - } > + mount_notify_automount(m, old_state, state); > > if (state != old_state) > log_unit_debug(UNIT(m)->id, > diff --git a/src/fstab-generator/fstab-generator.c > b/src/fstab-generator/fstab-generator.c > index 2ece12f79202..0b34f2fa3c43 100644 > --- a/src/fstab-generator/fstab-generator.c > +++ b/src/fstab-generator/fstab-generator.c > @@ -154,6 +154,29 @@ static bool mount_in_initrd(struct mntent *me) { > streq(me->mnt_dir, "/usr"); > } > > +static int write_idle_timeout(FILE *f, const char *where, const char *opts, > char **filtered) { > + _cleanup_free_ char *timeout = NULL; > + usec_t u; > + int r; > + > + r = fstab_filter_options(opts, "x-systemd.idle-timeout\0", > + NULL, &timeout, filtered); > + if (r <= 0) > + return r; > + > + r = parse_sec(timeout, &u); > + if (r < 0) { > + log_warning("Failed to parse timeout for %s, ignoring: %s", > + where, timeout); > + return 0; > + } > + > + fprintf(f, > + "TimeoutIdleSec=" USEC_FMT "\n", > + u / USEC_PER_SEC); > + > + return 0; > +} > static int add_mount( > const char *what, > const char *where, > @@ -293,6 +316,10 @@ static int add_mount( > "Where=%s\n", > where); > > + r = write_idle_timeout(f, where, opts, &filtered); > + if (r < 0) > + return r; > + > fflush(f); > if (ferror(f)) > return log_error_errno(errno, "Failed to write unit > file %s: %m", automount_unit); > -- > 2.1.4 > > _______________________________________________ > systemd-devel mailing list > systemd-devel@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/systemd-devel _______________________________________________ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel