From cd53c8f97d6247a7c73b0bde32789581bf444b3d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Thu, 13 Dec 2018 15:41:09 +0100 Subject: [PATCH 1/8] sd-device: attempt to read db again if it wasn't found This mostly reverts "sd-device: don't retry loading uevent/db files more than once", 7141e4f62c3f220872df3114c42d9e4b9525e43e. We will retry if we couldn't access the file, but not if parsing failed. Not re-reading the database at all just doesn't seem like a good idea. We have two implementations of device_read_db, and one does that, and the other retries to read the db. Re-reading seems more useful, since we can create the object and then access properties as some later time when we know that the device has been initialized and we can get useful results. Otherwise, we force the user to destroy this object and create a new one. This changes device_read_uevent_file() and device_read_db_aux(). See next commit for description of where those functions are used. --- src/libsystemd/sd-device/sd-device.c | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/src/libsystemd/sd-device/sd-device.c b/src/libsystemd/sd-device/sd-device.c index d5583488f20..95068afd963 100644 --- a/src/libsystemd/sd-device/sd-device.c +++ b/src/libsystemd/sd-device/sd-device.c @@ -498,8 +498,6 @@ int device_read_uevent_file(sd_device *device) { if (device->uevent_loaded || device->sealed) return 0; - device->uevent_loaded = true; - r = sd_device_get_syspath(device, &syspath); if (r < 0) return r; @@ -507,15 +505,19 @@ int device_read_uevent_file(sd_device *device) { path = strjoina(syspath, "/uevent"); r = read_full_file(path, &uevent, &uevent_len); - if (r == -EACCES) + if (r == -EACCES) { /* empty uevent files may be write-only */ + device->uevent_loaded = true; return 0; - else if (r == -ENOENT) + } + if (r == -ENOENT) /* some devices may not have uevent files, see set_syspath() */ return 0; - else if (r < 0) + if (r < 0) return log_device_debug_errno(device, r, "sd-device: Failed to read uevent file '%s': %m", path); + device->uevent_loaded = true; + for (i = 0; i < uevent_len; i++) switch (state) { case PRE_KEY: @@ -1301,8 +1303,6 @@ int device_read_db_aux(sd_device *device, bool force) { if (device->db_loaded || (!force && device->sealed)) return 0; - device->db_loaded = true; - r = device_get_id_filename(device, &id); if (r < 0) return r; @@ -1320,6 +1320,8 @@ int device_read_db_aux(sd_device *device, bool force) { /* devices with a database entry are initialized */ device->is_initialized = true; + device->db_loaded = true; + for (i = 0; i < db_len; i++) { switch (state) { case PRE_KEY: From ebcc52fad68daf4bb3198685c7ab730813606fff Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Thu, 13 Dec 2018 15:49:49 +0100 Subject: [PATCH 2/8] sd-device: reduce the number of implementations of device_read_db() we keep around MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit We had two very similar functions: device_read_db_aux and device_read_db, and a number of wrappers for them: device_read_db_aux ← device_read_db (in sd-device.c) ← all functions in sd-device.c, including sd_device_is_initialized ← device_read_db_force ← event_execute_rules_on_remove (in udev-event.c) device_read_db (in device-private.c) ← functions in device_private.c (but not device_read_db_force): device_get_devnode_{mode,uid,gid} device_get_devlink_priority device_get_watch_handle device_clone_with_db ← called from udevadm, udev-{node,event,watch}.c Before 7141e4f62c3f220872df3114c42d9e4b9525e43e (sd-device: don't retry loading uevent/db files more than once), the two implementations were the same. In that commit, device_read_db_aux was changed. Those changes were reverted in the parent commit, so the two implementations are now again the same except for superficial differences. This commit removes device_read_db (in sd-device.c), and renames device_read_db_aux to device_read_db_internal and makes everyone use this one implementation. There should be no functional change. --- src/libsystemd/sd-device/device-internal.h | 1 - src/libsystemd/sd-device/device-private.c | 174 --------------------- src/libsystemd/sd-device/device-private.h | 5 +- src/libsystemd/sd-device/sd-device.c | 8 +- src/udev/udev-event.c | 2 +- 5 files changed, 8 insertions(+), 182 deletions(-) diff --git a/src/libsystemd/sd-device/device-internal.h b/src/libsystemd/sd-device/device-internal.h index 39e984cf7cf..2a9e1a789b1 100644 --- a/src/libsystemd/sd-device/device-internal.h +++ b/src/libsystemd/sd-device/device-internal.h @@ -97,7 +97,6 @@ int device_new_aux(sd_device **ret); int device_add_property_aux(sd_device *device, const char *key, const char *value, bool db); int device_add_property_internal(sd_device *device, const char *key, const char *value); int device_read_uevent_file(sd_device *device); -int device_read_db_aux(sd_device *device, bool force); int device_set_syspath(sd_device *device, const char *_syspath, bool verify); int device_set_ifindex(sd_device *device, const char *ifindex); diff --git a/src/libsystemd/sd-device/device-private.c b/src/libsystemd/sd-device/device-private.c index 95a581e66ef..b1b6f6f20ac 100644 --- a/src/libsystemd/sd-device/device-private.c +++ b/src/libsystemd/sd-device/device-private.c @@ -47,81 +47,6 @@ int device_add_property(sd_device *device, const char *key, const char *value) { return 0; } -static int device_add_property_internal_from_string(sd_device *device, const char *str) { - _cleanup_free_ char *key = NULL; - char *value; - - assert(device); - assert(str); - - key = strdup(str); - if (!key) - return -ENOMEM; - - value = strchr(key, '='); - if (!value) - return -EINVAL; - - *value = '\0'; - - if (isempty(++value)) - value = NULL; - - return device_add_property_internal(device, key, value); -} - -static int handle_db_line(sd_device *device, char key, const char *value) { - char *path; - int r; - - assert(device); - assert(value); - - switch (key) { - case 'S': - path = strjoina("/dev/", value); - r = device_add_devlink(device, path); - if (r < 0) - return r; - - break; - case 'L': - r = safe_atoi(value, &device->devlink_priority); - if (r < 0) - return r; - - break; - case 'E': - r = device_add_property_internal_from_string(device, value); - if (r < 0) - return r; - - break; - case 'G': - r = device_add_tag(device, value); - if (r < 0) - return r; - - break; - case 'W': - r = safe_atoi(value, &device->watch_handle); - if (r < 0) - return r; - - break; - case 'I': - r = device_set_usec_initialized(device, value); - if (r < 0) - return r; - - break; - default: - log_device_debug(device, "sd-device: Unknown key '%c' in device db, ignoring", key); - } - - return 0; -} - void device_set_devlink_priority(sd_device *device, int priority) { assert(device); @@ -157,99 +82,6 @@ int device_ensure_usec_initialized(sd_device *device, sd_device *device_old) { return 0; } -static int device_read_db(sd_device *device) { - _cleanup_free_ char *db = NULL; - char *path; - const char *id, *value; - char key; - size_t db_len; - unsigned i; - int r; - - enum { - PRE_KEY, - KEY, - PRE_VALUE, - VALUE, - INVALID_LINE, - } state = PRE_KEY; - - assert(device); - - if (device->db_loaded || device->sealed) - return 0; - - r = device_get_id_filename(device, &id); - if (r < 0) - return r; - - path = strjoina("/run/udev/data/", id); - - r = read_full_file(path, &db, &db_len); - if (r < 0) { - if (r == -ENOENT) - return 0; - else - return log_device_debug_errno(device, r, "sd-device: Failed to read db '%s': %m", path); - } - - /* devices with a database entry are initialized */ - device_set_is_initialized(device); - - for (i = 0; i < db_len; i++) { - switch (state) { - case PRE_KEY: - if (!strchr(NEWLINE, db[i])) { - key = db[i]; - - state = KEY; - } - - break; - case KEY: - if (db[i] != ':') { - log_device_debug(device, "sd-device: Invalid db entry with key '%c', ignoring", key); - - state = INVALID_LINE; - } else { - db[i] = '\0'; - - state = PRE_VALUE; - } - - break; - case PRE_VALUE: - value = &db[i]; - - state = VALUE; - - break; - case INVALID_LINE: - if (strchr(NEWLINE, db[i])) - state = PRE_KEY; - - break; - case VALUE: - if (strchr(NEWLINE, db[i])) { - db[i] = '\0'; - r = handle_db_line(device, key, value); - if (r < 0) - log_device_debug_errno(device, r, "sd-device: Failed to handle db entry '%c:%s', ignoring: %m", key, value); - - state = PRE_KEY; - } - - break; - default: - assert_not_reached("Invalid state when parsing db"); - } - } - - device->db_loaded = true; - - return 0; -} - uint64_t device_get_properties_generation(sd_device *device) { assert(device); @@ -1115,9 +947,3 @@ int device_delete_db(sd_device *device) { return 0; } - -int device_read_db_force(sd_device *device) { - assert(device); - - return device_read_db_aux(device, true); -} diff --git a/src/libsystemd/sd-device/device-private.h b/src/libsystemd/sd-device/device-private.h index 7e4a78e0672..56558b38c6f 100644 --- a/src/libsystemd/sd-device/device-private.h +++ b/src/libsystemd/sd-device/device-private.h @@ -49,4 +49,7 @@ int device_new_from_synthetic_event(sd_device **new_device, const char *syspath, int device_tag_index(sd_device *dev, sd_device *dev_old, bool add); int device_update_db(sd_device *device); int device_delete_db(sd_device *device); -int device_read_db_force(sd_device *device); +int device_read_db_internal(sd_device *device, bool force); +static inline int device_read_db(sd_device *device) { + return device_read_db_internal(device, false); +} diff --git a/src/libsystemd/sd-device/sd-device.c b/src/libsystemd/sd-device/sd-device.c index 95068afd963..6a1c7ff0992 100644 --- a/src/libsystemd/sd-device/sd-device.c +++ b/src/libsystemd/sd-device/sd-device.c @@ -1283,7 +1283,7 @@ int device_get_id_filename(sd_device *device, const char **ret) { return 0; } -int device_read_db_aux(sd_device *device, bool force) { +int device_read_db_internal(sd_device *device, bool force) { _cleanup_free_ char *db = NULL; char *path; const char *id, *value; @@ -1300,6 +1300,8 @@ int device_read_db_aux(sd_device *device, bool force) { INVALID_LINE, } state = PRE_KEY; + assert(device); + if (device->db_loaded || (!force && device->sealed)) return 0; @@ -1374,10 +1376,6 @@ int device_read_db_aux(sd_device *device, bool force) { return 0; } -static int device_read_db(sd_device *device) { - return device_read_db_aux(device, false); -} - _public_ int sd_device_get_is_initialized(sd_device *device) { int r; diff --git a/src/udev/udev-event.c b/src/udev/udev-event.c index bb3e1b9f238..a2d1b47dbe7 100644 --- a/src/udev/udev-event.c +++ b/src/udev/udev-event.c @@ -785,7 +785,7 @@ static void event_execute_rules_on_remove( sd_device *dev = event->dev; int r; - r = device_read_db_force(dev); + r = device_read_db_internal(dev, true); if (r < 0) log_device_debug_errno(dev, r, "Failed to read database under /run/udev/data/: %m"); From dc5042c0a34faa2cb01206141f819141b25d55b0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Fri, 14 Dec 2018 11:33:17 +0100 Subject: [PATCH 3/8] sd-device: pass timestamp internally as usec_t not char* --- src/libsystemd/sd-device/device-internal.h | 3 ++- src/libsystemd/sd-device/device-private.c | 26 +++++++++----------- src/libsystemd/sd-device/sd-device.c | 28 ++++++++++++---------- 3 files changed, 29 insertions(+), 28 deletions(-) diff --git a/src/libsystemd/sd-device/device-internal.h b/src/libsystemd/sd-device/device-internal.h index 2a9e1a789b1..0a0a24aa8ee 100644 --- a/src/libsystemd/sd-device/device-internal.h +++ b/src/libsystemd/sd-device/device-internal.h @@ -5,6 +5,7 @@ #include "hashmap.h" #include "set.h" +#include "time-util.h" struct sd_device { unsigned n_ref; @@ -106,7 +107,7 @@ int device_set_devtype(sd_device *device, const char *_devtype); int device_set_devnum(sd_device *device, const char *major, const char *minor); int device_set_subsystem(sd_device *device, const char *_subsystem); int device_set_driver(sd_device *device, const char *_driver); -int device_set_usec_initialized(sd_device *device, const char *initialized); +int device_set_usec_initialized(sd_device *device, usec_t when); DeviceAction device_action_from_string(const char *s) _pure_; const char *device_action_to_string(DeviceAction a) _const_; diff --git a/src/libsystemd/sd-device/device-private.c b/src/libsystemd/sd-device/device-private.c index b1b6f6f20ac..36beb3e7dfa 100644 --- a/src/libsystemd/sd-device/device-private.c +++ b/src/libsystemd/sd-device/device-private.c @@ -60,26 +60,16 @@ void device_set_is_initialized(sd_device *device) { } int device_ensure_usec_initialized(sd_device *device, sd_device *device_old) { - char num[DECIMAL_STR_MAX(usec_t)]; - usec_t usec_initialized; - int r; + usec_t when; assert(device); if (device_old && device_old->usec_initialized > 0) - usec_initialized = device_old->usec_initialized; + when = device_old->usec_initialized; else - usec_initialized = now(CLOCK_MONOTONIC); + when = now(CLOCK_MONOTONIC); - r = snprintf(num, sizeof(num), USEC_FMT, usec_initialized); - if (r < 0) - return -errno; - - r = device_set_usec_initialized(device, num); - if (r < 0) - return r; - - return 0; + return device_set_usec_initialized(device, when); } uint64_t device_get_properties_generation(sd_device *device) { @@ -223,7 +213,13 @@ static int device_amend(sd_device *device, const char *key, const char *value) { if (r < 0) return log_device_debug_errno(device, r, "sd-device: Failed to set devname to '%s': %m", value); } else if (streq(key, "USEC_INITIALIZED")) { - r = device_set_usec_initialized(device, value); + usec_t t; + + r = safe_atou64(value, &t); + if (r < 0) + return log_device_debug_errno(device, r, "sd-device: Failed to parse timestamp '%s': %m", value); + + r = device_set_usec_initialized(device, t); if (r < 0) return log_device_debug_errno(device, r, "sd-device: Failed to set usec-initialized to '%s': %m", value); } else if (streq(key, "DRIVER")) { diff --git a/src/libsystemd/sd-device/sd-device.c b/src/libsystemd/sd-device/sd-device.c index 6a1c7ff0992..db58615df53 100644 --- a/src/libsystemd/sd-device/sd-device.c +++ b/src/libsystemd/sd-device/sd-device.c @@ -22,6 +22,7 @@ #include "set.h" #include "socket-util.h" #include "stat-util.h" +#include "stdio-util.h" #include "string-util.h" #include "strv.h" #include "strxcpyx.h" @@ -1149,23 +1150,19 @@ static int device_add_property_internal_from_string(sd_device *device, const cha return device_add_property_internal(device, key, value); } -int device_set_usec_initialized(sd_device *device, const char *initialized) { - uint64_t usec_initialized; +int device_set_usec_initialized(sd_device *device, usec_t when) { + char s[DECIMAL_STR_MAX(usec_t)]; int r; assert(device); - assert(initialized); - r = safe_atou64(initialized, &usec_initialized); + xsprintf(s, USEC_FMT, when); + + r = device_add_property_internal(device, "USEC_INITIALIZED", s); if (r < 0) return r; - r = device_add_property_internal(device, "USEC_INITIALIZED", initialized); - if (r < 0) - return r; - - device->usec_initialized = usec_initialized; - + device->usec_initialized = when; return 0; } @@ -1196,12 +1193,19 @@ static int handle_db_line(sd_device *device, char key, const char *value) { return r; break; - case 'I': - r = device_set_usec_initialized(device, value); + case 'I': { + usec_t t; + + r = safe_atou64(value, &t); + if (r < 0) + return r; + + r = device_set_usec_initialized(device, t); if (r < 0) return r; break; + } case 'L': r = safe_atoi(value, &device->devlink_priority); if (r < 0) From 11c49e6df563e264e6094c4d521c3a2dc8e089b3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Mon, 17 Dec 2018 12:29:28 +0100 Subject: [PATCH 4/8] sd-device: remove holes in struct sd_device Normally, we don't care too much about what pahole reports. But this structure could potentially be allocated for every device on the system, i.e. in a large number of copies. 5 vs 7 cache lines is nice. /* size: 400, cachelines: 7, members: 53 */ /* sum members: 330, holes: 12, sum holes: 70 */ /* last cacheline: 16 bytes */ /* size: 320, cachelines: 5, members: 53 */ /* bit holes: 1, sum bit holes: 6 bits */ /* bit_padding: 5 bits */ --- src/libsystemd/sd-device/device-internal.h | 40 +++++++++++----------- 1 file changed, 20 insertions(+), 20 deletions(-) diff --git a/src/libsystemd/sd-device/device-internal.h b/src/libsystemd/sd-device/device-internal.h index 0a0a24aa8ee..3ffca35cbef 100644 --- a/src/libsystemd/sd-device/device-internal.h +++ b/src/libsystemd/sd-device/device-internal.h @@ -10,8 +10,9 @@ struct sd_device { unsigned n_ref; + int watch_handle; + sd_device *parent; - bool parent_set; /* no need to try to reload parent */ OrderedHashmap *properties; Iterator properties_iterator; @@ -25,60 +26,59 @@ struct sd_device { Set *sysattrs; /* names of sysattrs */ Iterator sysattrs_iterator; - bool sysattrs_read; /* don't try to re-read sysattrs once read */ Set *tags; Iterator tags_iterator; uint64_t tags_generation; /* changes whenever the tags are changed */ uint64_t tags_iterator_generation; /* generation when iteration was started */ - bool property_tags_outdated; /* need to update TAGS= property */ Set *devlinks; Iterator devlinks_iterator; uint64_t devlinks_generation; /* changes whenever the devlinks are changed */ uint64_t devlinks_iterator_generation; /* generation when iteration was started */ - bool property_devlinks_outdated; /* need to update DEVLINKS= property */ int devlink_priority; + int ifindex; + char *devtype; + char *devname; + dev_t devnum; + char **properties_strv; /* the properties hashmap as a strv */ uint8_t *properties_nulstr; /* the same as a nulstr */ size_t properties_nulstr_len; - bool properties_buf_outdated; /* need to reread hashmap */ - - int watch_handle; char *syspath; const char *devpath; const char *sysnum; char *sysname; - bool sysname_set; /* don't reread sysname */ - - char *devtype; - int ifindex; - char *devname; - dev_t devnum; char *subsystem; - bool subsystem_set; /* don't reread subsystem */ char *driver_subsystem; /* only set for the 'drivers' subsystem */ - bool driver_subsystem_set; /* don't reread subsystem */ char *driver; - bool driver_set; /* don't reread driver */ char *id_filename; - bool is_initialized; uint64_t usec_initialized; mode_t devmode; uid_t devuid; gid_t devgid; - bool uevent_loaded; /* don't reread uevent */ + bool parent_set:1; /* no need to try to reload parent */ + bool sysattrs_read:1; /* don't try to re-read sysattrs once read */ + bool property_tags_outdated:1; /* need to update TAGS= property */ + bool property_devlinks_outdated:1; /* need to update DEVLINKS= property */ + bool properties_buf_outdated:1; /* need to reread hashmap */ + bool sysname_set:1; /* don't reread sysname */ + bool subsystem_set:1; /* don't reread subsystem */ + bool driver_subsystem_set:1; /* don't reread subsystem */ + bool driver_set:1; /* don't reread driver */ + bool uevent_loaded:1; /* don't reread uevent */ bool db_loaded; /* don't reread db */ - bool sealed; /* don't read more information from uevent/db */ - bool db_persist; /* don't clean up the db when switching from initrd to real root */ + bool is_initialized:1; + bool sealed:1; /* don't read more information from uevent/db */ + bool db_persist:1; /* don't clean up the db when switching from initrd to real root */ }; typedef enum DeviceAction { From ed435031a599335a003f17eac45af5696ffc3a86 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Fri, 14 Dec 2018 12:32:33 +0100 Subject: [PATCH 5/8] rfkill: move wait_for_initialized() to shared/ The function interface is the same, except that the output pointer may be NULL. The implementation is slightly simplified by taking advantage of changes in ancestor commit 'sd-device: attempt to read db again if it wasn't found', by not creating a new sd_device object before re-checking the is_initialized status. v2: - In v1, the old object was always used and the device received back from the sd_device_monitor_start callback was ignored. I *think* the result will be equivalent in both cases, because by the time we the callback gets called, the db entry in the filesystem will also exist, and any subsequent access to properties of the object would trigger a read of the database from disk. But I'm not certain, and anyway, using the device object received in the callback seems cleaner. --- src/rfkill/rfkill.c | 86 +----------------------------------------- src/shared/udev-util.c | 78 ++++++++++++++++++++++++++++++++++++++ src/shared/udev-util.h | 4 ++ 3 files changed, 84 insertions(+), 84 deletions(-) diff --git a/src/rfkill/rfkill.c b/src/rfkill/rfkill.c index 1cc887df122..ac21dc064ca 100644 --- a/src/rfkill/rfkill.c +++ b/src/rfkill/rfkill.c @@ -18,6 +18,7 @@ #include "proc-cmdline.h" #include "string-table.h" #include "string-util.h" +#include "udev-util.h" #include "util.h" #include "list.h" @@ -88,89 +89,6 @@ static int find_device( return 0; } -struct DeviceMonitorData { - const char *sysname; - sd_device *device; -}; - -static int device_monitor_handler(sd_device_monitor *monitor, sd_device *device, void *userdata) { - struct DeviceMonitorData *data = userdata; - const char *sysname; - - assert(device); - assert(data); - assert(data->sysname); - - if (sd_device_get_sysname(device, &sysname) >= 0 && streq(sysname, data->sysname)) { - data->device = sd_device_ref(device); - return sd_event_exit(sd_device_monitor_get_event(monitor), 0); - } - - return 0; -} - -static int wait_for_initialized( - sd_device *device, - sd_device **ret) { - - _cleanup_(sd_device_monitor_unrefp) sd_device_monitor *monitor = NULL; - _cleanup_(sd_event_unrefp) sd_event *event = NULL; - _cleanup_(sd_device_unrefp) sd_device *d = NULL; - struct DeviceMonitorData data = {}; - int r; - - assert(device); - assert(ret); - - if (sd_device_get_is_initialized(device) > 0) { - *ret = sd_device_ref(device); - return 0; - } - - assert_se(sd_device_get_sysname(device, &data.sysname) >= 0); - - /* Wait until the device is initialized, so that we can get - * access to the ID_PATH property */ - - r = sd_event_default(&event); - if (r < 0) - return log_error_errno(r, "Failed to get default event: %m"); - - r = sd_device_monitor_new(&monitor); - if (r < 0) - return log_error_errno(r, "Failed to acquire monitor: %m"); - - r = sd_device_monitor_filter_add_match_subsystem_devtype(monitor, "rfkill", NULL); - if (r < 0) - return log_error_errno(r, "Failed to add rfkill device match to monitor: %m"); - - r = sd_device_monitor_attach_event(monitor, event); - if (r < 0) - return log_error_errno(r, "Failed to attach event to device monitor: %m"); - - r = sd_device_monitor_start(monitor, device_monitor_handler, &data); - if (r < 0) - return log_error_errno(r, "Failed to start device monitor: %m"); - - /* Check again, maybe things changed */ - r = sd_device_new_from_subsystem_sysname(&d, "rfkill", data.sysname); - if (r < 0) - return log_full_errno(IN_SET(r, -ENOENT, -ENXIO, -ENODEV) ? LOG_DEBUG : LOG_ERR, r, - "Failed to open device '%s': %m", data.sysname); - - if (sd_device_get_is_initialized(d) > 0) { - *ret = TAKE_PTR(d); - return 0; - } - - r = sd_event_loop(event); - if (r < 0) - return log_error_errno(r, "Event loop failed: %m"); - - *ret = TAKE_PTR(data.device); - return 0; -} - static int determine_state_file( const struct rfkill_event *event, char **ret) { @@ -187,7 +105,7 @@ static int determine_state_file( if (r < 0) return r; - r = wait_for_initialized(d, &device); + r = device_wait_for_initialization(d, "rfkill", &device); if (r < 0) return r; diff --git a/src/shared/udev-util.c b/src/shared/udev-util.c index fdeaf8f613e..4200032b3b6 100644 --- a/src/shared/udev-util.c +++ b/src/shared/udev-util.c @@ -91,3 +91,81 @@ int udev_parse_config_full( return 0; } + +struct DeviceMonitorData { + const char *sysname; + sd_device *device; +}; + +static int device_monitor_handler(sd_device_monitor *monitor, sd_device *device, void *userdata) { + struct DeviceMonitorData *data = userdata; + const char *sysname; + + assert(device); + assert(data); + assert(data->sysname); + assert(!data->device); + + if (sd_device_get_sysname(device, &sysname) >= 0 && streq(sysname, data->sysname)) { + data->device = sd_device_ref(device); + return sd_event_exit(sd_device_monitor_get_event(monitor), 0); + } + + return 0; +} + +int device_wait_for_initialization(sd_device *device, const char *subsystem, sd_device **ret) { + _cleanup_(sd_device_monitor_unrefp) sd_device_monitor *monitor = NULL; + _cleanup_(sd_event_unrefp) sd_event *event = NULL; + struct DeviceMonitorData data = {}; + int r; + + assert(device); + assert(subsystem); + + if (sd_device_get_is_initialized(device) > 0) { + if (ret) + *ret = sd_device_ref(device); + return 0; + } + + assert_se(sd_device_get_sysname(device, &data.sysname) >= 0); + + /* Wait until the device is initialized, so that we can get access to the ID_PATH property */ + + r = sd_event_default(&event); + if (r < 0) + return log_error_errno(r, "Failed to get default event: %m"); + + r = sd_device_monitor_new(&monitor); + if (r < 0) + return log_error_errno(r, "Failed to acquire monitor: %m"); + + r = sd_device_monitor_filter_add_match_subsystem_devtype(monitor, subsystem, NULL); + if (r < 0) + return log_error_errno(r, "Failed to add %s subsystem match to monitor: %m", subsystem); + + r = sd_device_monitor_attach_event(monitor, event); + if (r < 0) + return log_error_errno(r, "Failed to attach event to device monitor: %m"); + + r = sd_device_monitor_start(monitor, device_monitor_handler, &data); + if (r < 0) + return log_error_errno(r, "Failed to start device monitor: %m"); + + /* Check again, maybe things changed. Udev will re-read the db if the device wasn't initialized + * yet. */ + if (sd_device_get_is_initialized(device) > 0) { + if (ret) + *ret = sd_device_ref(device); + return 0; + } + + r = sd_event_loop(event); + if (r < 0) + return log_error_errno(r, "Event loop failed: %m"); + + if (ret) + *ret = TAKE_PTR(data.device); + return 0; +} diff --git a/src/shared/udev-util.h b/src/shared/udev-util.h index be08fe1ff89..932c4a9cd5f 100644 --- a/src/shared/udev-util.h +++ b/src/shared/udev-util.h @@ -1,6 +1,8 @@ /* SPDX-License-Identifier: LGPL-2.1+ */ #pragma once +#include "sd-device.h" + #include "time-util.h" typedef enum ResolveNameTiming { @@ -23,3 +25,5 @@ int udev_parse_config_full( static inline int udev_parse_config(void) { return udev_parse_config_full(NULL, NULL, NULL, NULL); } + +int device_wait_for_initialization(sd_device *device, const char *subsystem, sd_device **ret); From ea887be00b7a9374e29bc91fcd3e8fa6ea507eac Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Thu, 13 Dec 2018 13:28:58 +0100 Subject: [PATCH 6/8] dissect-image: split out a chunk of dissect_image() out No functional change, just moving code around. --- src/shared/dissect-image.c | 204 +++++++++++++++++++++---------------- 1 file changed, 119 insertions(+), 85 deletions(-) diff --git a/src/shared/dissect-image.c b/src/shared/dissect-image.c index 6ea8e4df8d9..0147c45d8e0 100644 --- a/src/shared/dissect-image.c +++ b/src/shared/dissect-image.c @@ -116,6 +116,122 @@ static bool device_is_block(sd_device *d) { return streq(ss, "block"); } + +static int enumerator_for_parent(sd_device *d, sd_device_enumerator **ret) { + _cleanup_(sd_device_enumerator_unrefp) sd_device_enumerator *e = NULL; + int r; + + r = sd_device_enumerator_new(&e); + if (r < 0) + return r; + + r = sd_device_enumerator_allow_uninitialized(e); + if (r < 0) + return r; + + r = sd_device_enumerator_add_match_parent(e, d); + if (r < 0) + return r; + + *ret = TAKE_PTR(e); + return 0; +} + +/* how many times to wait for the device nodes to appear */ +#define N_DEVICE_NODE_LIST_ATTEMPTS 10 + +static int wait_for_partitions_to_appear( + int fd, + sd_device *d, + unsigned num_partitions, + sd_device_enumerator **ret_enumerator) { + + _cleanup_(sd_device_enumerator_unrefp) sd_device_enumerator *e = NULL; + sd_device *q; + unsigned n; + int r; + + r = enumerator_for_parent(d, &e); + if (r < 0) + return r; + + /* Count the partitions enumerated by the kernel */ + n = 0; + FOREACH_DEVICE(e, q) { + if (sd_device_get_devnum(q, NULL) < 0) + continue; + if (!device_is_block(q)) + continue; + if (device_is_mmc_special_partition(q)) + continue; + + n++; + } + + if (n == num_partitions + 1) { + *ret_enumerator = TAKE_PTR(e); + return 0; /* success! */ + } + if (n > num_partitions + 1) + return log_debug_errno(SYNTHETIC_ERRNO(EIO), + "blkid and kernel partition lists do not match."); + + /* The kernel has probed fewer partitions than blkid? Maybe the kernel prober is still running or it + * got EBUSY because udev already opened the device. Let's reprobe the device, which is a synchronous + * call that waits until probing is complete. */ + + for (unsigned j = 0; ; j++) { + if (j++ > 20) + return -EBUSY; + + if (ioctl(fd, BLKRRPART, 0) >= 0) + break; + r = -errno; + if (r == -EINVAL) { + struct loop_info64 info; + + /* If we are running on a loop device that has partition scanning off, return + * an explicit recognizable error about this, so that callers can generate a + * proper message explaining the situation. */ + + if (ioctl(fd, LOOP_GET_STATUS64, &info) >= 0 && (info.lo_flags & LO_FLAGS_PARTSCAN) == 0) { + log_debug("Device is a loop device and partition scanning is off!"); + return -EPROTONOSUPPORT; + } + } + if (r != -EBUSY) + return r; + + /* If something else has the device open, such as an udev rule, the ioctl will return + * EBUSY. Since there's no way to wait until it isn't busy anymore, let's just wait a bit, + * and try again. + * + * This is really something they should fix in the kernel! */ + (void) usleep(50 * USEC_PER_MSEC); + + } + + return -EAGAIN; /* no success yet, try again */ +} + +static int loop_wait_for_partitions_to_appear( + int fd, + sd_device *d, + unsigned num_partitions, + sd_device_enumerator **ret_enumerator) { + int r; + + for (unsigned i = 0; i < N_DEVICE_NODE_LIST_ATTEMPTS; i++) { + r = wait_for_partitions_to_appear(fd, d, num_partitions, ret_enumerator); + if (r != -EAGAIN) + return r; + } + + return log_debug_errno(SYNTHETIC_ERRNO(ENXIO), + "Kernel partitions dit not appear within %d attempts", + N_DEVICE_NODE_LIST_ATTEMPTS); +} + #endif int dissect_image( @@ -262,91 +378,9 @@ int dissect_image( if (r < 0) return r; - for (i = 0;; i++) { - int n, z; - - if (i >= 10) { - log_debug("Kernel partitions never appeared."); - return -ENXIO; - } - - r = sd_device_enumerator_new(&e); - if (r < 0) - return r; - - r = sd_device_enumerator_allow_uninitialized(e); - if (r < 0) - return r; - - r = sd_device_enumerator_add_match_parent(e, d); - if (r < 0) - return r; - - /* Count the partitions enumerated by the kernel */ - n = 0; - FOREACH_DEVICE(e, q) { - if (sd_device_get_devnum(q, NULL) < 0) - continue; - - if (!device_is_block(q)) - continue; - - if (device_is_mmc_special_partition(q)) - continue; - n++; - } - - /* Count the partitions enumerated by blkid */ - z = blkid_partlist_numof_partitions(pl); - if (n == z + 1) - break; - if (n > z + 1) { - log_debug("blkid and kernel partition list do not match."); - return -EIO; - } - if (n < z + 1) { - unsigned j = 0; - - /* The kernel has probed fewer partitions than blkid? Maybe the kernel prober is still running - * or it got EBUSY because udev already opened the device. Let's reprobe the device, which is a - * synchronous call that waits until probing is complete. */ - - for (;;) { - if (j++ > 20) - return -EBUSY; - - if (ioctl(fd, BLKRRPART, 0) < 0) { - r = -errno; - - if (r == -EINVAL) { - struct loop_info64 info; - - /* If we are running on a loop device that has partition scanning off, - * return an explicit recognizable error about this, so that callers - * can generate a proper message explaining the situation. */ - - if (ioctl(fd, LOOP_GET_STATUS64, &info) >= 0 && (info.lo_flags & LO_FLAGS_PARTSCAN) == 0) { - log_debug("Device is loop device and partition scanning is off!"); - return -EPROTONOSUPPORT; - } - } - if (r != -EBUSY) - return r; - } else - break; - - /* If something else has the device open, such as an udev rule, the ioctl will return - * EBUSY. Since there's no way to wait until it isn't busy anymore, let's just wait a - * bit, and try again. - * - * This is really something they should fix in the kernel! */ - - (void) usleep(50 * USEC_PER_MSEC); - } - } - - e = sd_device_enumerator_unref(e); - } + r = loop_wait_for_partitions_to_appear(fd, d, blkid_partlist_numof_partitions(pl), &e); + if (r < 0) + return r; FOREACH_DEVICE(e, q) { unsigned long long pflags; From b887c8b8a836963c2949ae6a11b77430886d0b77 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Thu, 13 Dec 2018 14:35:15 +0100 Subject: [PATCH 7/8] dissect-image: wait for the root to appear dissect-image would wait for the root device and paritions to appear. But if we had an image with no partitions, we'd not wait at all. If the kernel or udev were slow in creating device nodes or symlinks, subsequent mount attempt might fail if nspawn won the race. Calling wait_for_partitions_to_appear() in case of no partitions means that we verify that the kernel agrees that there are no partitions. We verify that the kernel sees the same number of partitions as blkid, so let's that also in this case. This makes the failure in #10526 much less likely, but doesn't eliminate it completely. Stay tuned. --- src/shared/dissect-image.c | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/src/shared/dissect-image.c b/src/shared/dissect-image.c index 0147c45d8e0..0e2fb9afbe6 100644 --- a/src/shared/dissect-image.c +++ b/src/shared/dissect-image.c @@ -221,6 +221,8 @@ static int loop_wait_for_partitions_to_appear( sd_device_enumerator **ret_enumerator) { int r; + log_debug("Waiting for device (parent + %d partitions) to appear...", num_partitions); + for (unsigned i = 0; i < N_DEVICE_NODE_LIST_ATTEMPTS; i++) { r = wait_for_partitions_to_appear(fd, d, num_partitions, ret_enumerator); if (r != -EAGAIN) @@ -320,6 +322,10 @@ int dissect_image( if (!m) return -ENOMEM; + r = sd_device_new_from_devnum(&d, 'b', st.st_rdev); + if (r < 0) + return r; + if (!(flags & DISSECT_IMAGE_GPT_ONLY) && (flags & DISSECT_IMAGE_REQUIRE_ROOT)) { const char *usage = NULL; @@ -353,6 +359,10 @@ int dissect_image( m->encrypted = streq_ptr(fstype, "crypto_LUKS"); + r = loop_wait_for_partitions_to_appear(fd, d, 0, &e); + if (r < 0) + return r; + *ret = TAKE_PTR(m); return 0; @@ -374,10 +384,6 @@ int dissect_image( if (!pl) return -errno ?: -ENOMEM; - r = sd_device_new_from_devnum(&d, 'b', st.st_rdev); - if (r < 0) - return r; - r = loop_wait_for_partitions_to_appear(fd, d, blkid_partlist_numof_partitions(pl), &e); if (r < 0) return r; From a8040b6d0ad43e5cb7168fd3b739c8c68aaa61b1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Thu, 13 Dec 2018 16:39:05 +0100 Subject: [PATCH 8/8] dissect-image: wait for the main device and all partitions to be known by udev MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fixes #10526. Even if we waited for the root device to appear, the mount could still fail if we didn't wait for udev to initalize the device. In particular, the /dev/block/n:m path used to mount the device is created by udev, and nspawn would sometimes win the race and the mount would fail with -ENOENT. The same wait is done for partitions, since if we try to mount them, the same considerations apply. Note: I first implemented a version which just does a loop (with a short wait). In that approach, udev takes on average ~800 µs to initialize the loopback device. The approach where we set up a monitor and avoid the loop is a bit nicer. There doesn't seem to be a significant difference in speed. With 1000 invocations of 'systemd-nspawn -i image.squashfs echo': loop (previous approach): real 4m52.625s user 0m37.094s sys 2m14.705s monitor (this patch): real 4m50.791s user 0m36.619s sys 2m14.039s --- src/shared/dissect-image.c | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/src/shared/dissect-image.c b/src/shared/dissect-image.c index 0e2fb9afbe6..4c2e41c8b8e 100644 --- a/src/shared/dissect-image.c +++ b/src/shared/dissect-image.c @@ -40,6 +40,7 @@ #include "string-util.h" #include "strv.h" #include "tmpfile-util.h" +#include "udev-util.h" #include "user-util.h" #include "xattr-util.h" @@ -165,6 +166,10 @@ static int wait_for_partitions_to_appear( if (device_is_mmc_special_partition(q)) continue; + r = device_wait_for_initialization(q, "block", NULL); + if (r < 0) + return r; + n++; } @@ -219,12 +224,17 @@ static int loop_wait_for_partitions_to_appear( sd_device *d, unsigned num_partitions, sd_device_enumerator **ret_enumerator) { + _cleanup_(sd_device_unrefp) sd_device *device = NULL; int r; log_debug("Waiting for device (parent + %d partitions) to appear...", num_partitions); + r = device_wait_for_initialization(d, "block", &device); + if (r < 0) + return r; + for (unsigned i = 0; i < N_DEVICE_NODE_LIST_ATTEMPTS; i++) { - r = wait_for_partitions_to_appear(fd, d, num_partitions, ret_enumerator); + r = wait_for_partitions_to_appear(fd, device, num_partitions, ret_enumerator); if (r != -EAGAIN) return r; }