Hello,
Lennart Poettering [2015-05-18 22:57 +0200]:
> > + 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?
Right, that was probably just the result of multiple edits/iterations;
moved back.
> > - 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?
Not sure, perhaps hysterical raisins? But..
> 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.
Makes sense, done now. It's an unrelated change to this patch, but if
you don't mind let's clean it up.
> > + } 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?
Right, done.
Updated patch attached. It doesn't look significantly smaller, mostly
because lots of it is an indentation change :/
Re-tested on a normal system, nspawn, LXC, and with ejecting a mounted
CD.
> > Subject: [PATCH 2/3] device: never transition from "tentative" to "dead"
> Not following on this patch
Will reply on your other response. TL/DR: Current code in master is
overzealous, this patch is overcautious, this needs some more thought
and work; I don't have an updated patch yet.
Martin
--
Martin Pitt | http://www.piware.de
Ubuntu Developer (www.ubuntu.com) | Debian Developer (www.debian.org)
From 8bbd9d1df6877867ce7958c2e51574b3e74c68e6 Mon Sep 17 00:00:00 2001 From: Martin Pitt <martin.p...@ubuntu.com> Date: Sun, 17 May 2015 15:07:47 +0200 Subject: [PATCH] device: create units with intended "found" value 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. Change device_setup_unit() to also accept a NULL udev_device, and don't add the extra udev information in that case. Previously device_found_node() would not create a .device unit, and unit_add_node_link() would then create a "dead" stub one via manager_load_unit(), so we lost the "found" attribute and unmounted everything from that device. https://launchpad.net/bugs/1444402 http://lists.freedesktop.org/archives/systemd-devel/2015-May/031658.html --- src/core/device.c | 51 ++++++++++++++++++++++++--------------------------- 1 file changed, 24 insertions(+), 27 deletions(-) diff --git a/src/core/device.c b/src/core/device.c index 8eca4c2..c784cab 100644 --- a/src/core/device.c +++ b/src/core/device.c @@ -292,18 +292,19 @@ static int device_add_udev_wants(Unit *u, struct udev_device *dev) { 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; + if (dev) { + sysfs = udev_device_get_syspath(dev); + if (!sysfs) + return 0; + } r = unit_name_from_path(path, ".device", &e); if (r < 0) @@ -312,6 +313,7 @@ static int device_setup_unit(Manager *m, struct udev_device *dev, const char *pa u = manager_get_unit(m, e); if (u && + sysfs && DEVICE(u)->sysfs && !path_equal(DEVICE(u)->sysfs, sysfs)) { log_unit_debug(u, "Device %s appeared twice with different sysfs paths %s and %s", e, DEVICE(u)->sysfs, sysfs); @@ -336,17 +338,19 @@ static int device_setup_unit(Manager *m, struct udev_device *dev, const char *pa /* If this was created via some dependency and has not * actually been seen yet ->sysfs will not be * initialized. Hence initialize it if necessary. */ + if (sysfs) { + r = device_set_sysfs(DEVICE(u), sysfs); + if (r < 0) + goto fail; - r = device_set_sysfs(DEVICE(u), sysfs); - if (r < 0) - goto fail; + (void) device_update_description(u, dev, path); - (void) device_update_description(u, dev, path); + /* The additional systemd udev properties we only interpret + * for the main object */ + if (main) + (void) device_add_udev_wants(u, dev); + } - /* The additional systemd udev properties we only interpret - * for the main object */ - if (main) - (void) device_add_udev_wants(u, dev); /* Note that this won't dispatch the load queue, the caller * has to do that if needed and appropriate */ @@ -788,23 +792,16 @@ int device_found_node(Manager *m, const char *node, bool add, DeviceFound found, * will still have a device node even when the medium * is not there... */ - 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) { - if (errno == ENOENT) - 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_error_errno(errno, "Failed to get udev device from devnum %u:%u: %m", major(st.st_rdev), minor(st.st_rdev)); - return log_oom(); - } + } else if (errno != ENOENT) + return log_error_errno(errno, "Failed to stat device node file %s: %m", node); /* If the device is known in the kernel and newly * appeared, then we'll create a device unit for it, -- 2.1.4
signature.asc
Description: Digital signature
_______________________________________________ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
