On Mon, 18.05.15 16:08, Martin Pitt (martin.p...@ubuntu.com) wrote:
> Change device_found_node() to also create a .device unit if a device is not
> known by udev; this is the case for "tentative" devices picked up by mountinfo
> (DEVICE_FOUND_MOUNT). With that we can record the "found" attribute on the
> unit.
Ah, I see now. This makes sense!
> static int device_setup_unit(Manager *m, struct udev_device *dev, const char
> *path, bool main) {
> _cleanup_free_ char *e = NULL;
> - const char *sysfs;
> + const char *sysfs = NULL;
> Unit *u = NULL;
> bool delete;
> int r;
>
> assert(m);
> - assert(dev);
> assert(path);
>
> - sysfs = udev_device_get_syspath(dev);
> - if (!sysfs)
> - return 0;
> -
> r = unit_name_from_path(path, ".device", &e);
> if (r < 0)
> return log_error_errno(r, "Failed to generate unit name from
> device path: %m");
>
> u = manager_get_unit(m, e);
>
> + if (dev) {
> + sysfs = udev_device_get_syspath(dev);
> + if (!sysfs)
> + return 0;
> + }
Why move this down? In order to keep the patch small and easy to grok,
can this stay up where it was, but simply be enclosed in the "if (dev) {}"
check?
>
> - if (stat(node, &st) < 0) {
> - if (errno == ENOENT)
> + if (stat(node, &st) >= 0) {
> + if (!S_ISBLK(st.st_mode) && !S_ISCHR(st.st_mode))
> return 0;
>
> - return log_error_errno(errno, "Failed to stat device
> node file %s: %m", node);
> - }
> -
> - if (!S_ISBLK(st.st_mode) && !S_ISCHR(st.st_mode))
> - return 0;
> + dev = udev_device_new_from_devnum(m->udev,
> S_ISBLK(st.st_mode) ? 'b' : 'c', st.st_rdev);
> + if (!dev && errno != ENOENT)
> + return log_oom();
Hmm, why are all these errors supposed to be OOM?
udev_device_new_from_devnum sets errno correctly, hence we should just
print what it really is set to with log_error_errno(), unless of
course it is ENOENT.
> - dev = udev_device_new_from_devnum(m->udev,
> S_ISBLK(st.st_mode) ? 'b' : 'c', st.st_rdev);
> - if (!dev) {
> - if (errno == ENOENT)
> - return 0;
> -
> - return log_oom();
> + } else {
> + if (errno != ENOENT)
> + return log_error_errno(errno, "Failed to
> stat device node file %s: %m", node);
The if "else {" and "if (errn..." lines could be merged into one
"else if (errno != ...", no?
> From c18dbd432ecd16c57123b5fc04313d625e8a8e88 Mon Sep 17 00:00:00 2001
> From: Martin Pitt <martin.p...@ubuntu.com>
> Date: Sun, 17 May 2015 15:17:58 +0200
> Subject: [PATCH 2/3] device: never transition from "tentative" to "dead"
>
> Only set a device to state "dead" if it was previously plugged through udev.
> We
> must never transition from "tentative" to "dead", as there is no way to be
> sure
> that this is actually true.
>
> This fixes systemd unmounting everything from a tentative device as soon as
> mountinfo changes.
Not following on this patch: what's the rationale here? your patch
basically means we would never ever clean up "tentative" devices, if
they once were referenced in mountinfo or /proc/swaps, but no longer
are.
Can you elaborate on what this patch is supposed to achieve? How could
it be problematic to deactivate device units that are neither
announced anywhere in /proc nor in udev?
Lennart
--
Lennart Poettering, Red Hat
_______________________________________________
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel
