[systemd-devel] [correct PATCH v2] dev-root.device is not active, results in an umount spree

[Lennart Poettering]

On Mon, 18.05.15 16:08, Martin Pitt (martin.pitt at 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.pitt at 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