On Thu, Apr 02, 2015 at 12:54:00PM +0200, Lennart Poettering wrote: > On Sun, 22.03.15 13:36, Michael Olbrich (m.olbr...@pengutronix.de) wrote: > > Love this work!
Thanks. [...] > > + > > + 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); > > +} > > Maybe the 5 * USEC_PER_SEC should become a #define somewhere at the > top of the file? Given that this is used more than once it might be > advisable to use the same definition. I've been thinking about this again and I changed this. Checking every 5 Seconds is rather often for longer timeouts. I've changed it to MAX(a->timeout_idle_usec/10, USEC_PER_SEC). I think that's a good compromise. > > + > > + r = set_ensure_allocated(&a->expire_tokens, NULL); > > + if (r < 0) { > > + log_unit_error(UNIT(a)->id, "Failed to > > allocate token set."); > > Sounds like a job for log_oom(). It's basically a copy of the code right above it, so it should match. I think that should be a separate patch. > > + 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); > > We normally don't like break this eagerly. Actually we do. In this file at least. The manager_add_job for JOB_START has the same line break :-). But I don't care either way, so I changed it. > > + return 0; > > + } > > + > > + fprintf(f, > > + "TimeoutIdleSec=" USEC_FMT "\n", > > + u / USEC_PER_SEC); > > + > > + return 0; > > Why not format that using format_timespan() here, so that the accuracy > is not lost? (We'll lose it when passing it to the kernel ultimately, > but we shouldn't lose it any earlier. In particular since you round up > when passing it to the kernel, but are rounding down here... Hence, > let's keep this simple, and always pass our native accuracy along > until the last point where we really have to convert it. Sounds reasonable, so I changed it. I basically copied this from the x-systemd.device-timeout implementation, so you might want to change that as well. I've changed everything else as requested. New patch follows. Michael -- Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 | _______________________________________________ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel