From 4dbce717873000cff7b56f89266d1d2fe53f9284 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Sun, 6 Dec 2020 20:10:48 +0900 Subject: [PATCH 1/4] set: introduce set_strjoin() --- src/basic/hashmap.c | 35 +++++++++++++++++++++++++++++++ src/basic/set.h | 2 ++ src/test/test-set.c | 51 +++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 88 insertions(+) diff --git a/src/basic/hashmap.c b/src/basic/hashmap.c index dd1b18c878..e38e580530 100644 --- a/src/basic/hashmap.c +++ b/src/basic/hashmap.c @@ -1976,3 +1976,38 @@ IteratedCache* iterated_cache_free(IteratedCache *cache) { return mfree(cache); } + +int set_strjoin(Set *s, const char *separator, char **ret) { + size_t separator_len, allocated = 0, len = 0; + _cleanup_free_ char *str = NULL; + const char *value; + bool first = true; + + assert(ret); + + separator_len = strlen_ptr(separator); + + SET_FOREACH(value, s) { + size_t l = strlen_ptr(value); + + if (l == 0) + continue; + + if (!GREEDY_REALLOC(str, allocated, len + l + (first ? 0 : separator_len) + 1)) + return -ENOMEM; + + if (separator_len > 0 && !first) { + memcpy(str + len, separator, separator_len); + len += separator_len; + } + + memcpy(str + len, value, l); + len += l; + first = false; + } + if (str) + str[len] = '\0'; + + *ret = TAKE_PTR(str); + return 0; +} diff --git a/src/basic/set.h b/src/basic/set.h index 20abc8f0dc..2b06c39cbe 100644 --- a/src/basic/set.h +++ b/src/basic/set.h @@ -150,3 +150,5 @@ DEFINE_TRIVIAL_CLEANUP_FUNC(Set*, set_free_free); #define _cleanup_set_free_ _cleanup_(set_freep) #define _cleanup_set_free_free_ _cleanup_(set_free_freep) + +int set_strjoin(Set *s, const char *separator, char **ret); diff --git a/src/test/test-set.c b/src/test/test-set.c index 16314d051b..8979408242 100644 --- a/src/test/test-set.c +++ b/src/test/test-set.c @@ -150,6 +150,56 @@ static void test_set_ensure_consume(void) { assert_se(set_size(m) == 2); } +static void test_set_strjoin(void) { + _cleanup_set_free_ Set *m = NULL; + _cleanup_free_ char *joined = NULL; + + assert_se(set_strjoin(m, NULL, &joined) >= 0); + assert_se(!joined); + assert_se(set_strjoin(m, "", &joined) >= 0); + assert_se(!joined); + assert_se(set_strjoin(m, " ", &joined) >= 0); + assert_se(!joined); + assert_se(set_strjoin(m, "xxx", &joined) >= 0); + assert_se(!joined); + + assert_se(set_put_strdup(&m, "aaa") == 1); + + assert_se(set_strjoin(m, NULL, &joined) >= 0); + assert_se(streq(joined, "aaa")); + + joined = mfree(joined); + assert_se(set_strjoin(m, "", &joined) >= 0); + assert_se(streq(joined, "aaa")); + + joined = mfree(joined); + assert_se(set_strjoin(m, " ", &joined) >= 0); + assert_se(streq(joined, "aaa")); + + joined = mfree(joined); + assert_se(set_strjoin(m, "xxx", &joined) >= 0); + assert_se(streq(joined, "aaa")); + + assert_se(set_put_strdup(&m, "bbb") == 1); + assert_se(set_put_strdup(&m, "aaa") == 0); + + joined = mfree(joined); + assert_se(set_strjoin(m, NULL, &joined) >= 0); + assert_se(STR_IN_SET(joined, "aaabbb", "bbbaaa")); + + joined = mfree(joined); + assert_se(set_strjoin(m, "", &joined) >= 0); + assert_se(STR_IN_SET(joined, "aaabbb", "bbbaaa")); + + joined = mfree(joined); + assert_se(set_strjoin(m, " ", &joined) >= 0); + assert_se(STR_IN_SET(joined, "aaa bbb", "bbb aaa")); + + joined = mfree(joined); + assert_se(set_strjoin(m, "xxx", &joined) >= 0); + assert_se(STR_IN_SET(joined, "aaaxxxbbb", "bbbxxxaaa")); +} + int main(int argc, const char *argv[]) { test_set_steal_first(); test_set_free_with_destructor(); @@ -160,6 +210,7 @@ int main(int argc, const char *argv[]) { test_set_ensure_allocated(); test_set_ensure_put(); test_set_ensure_consume(); + test_set_strjoin(); return 0; } From 6f3ac0d51766b0b9101676cefe5c4ba81feba436 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Sun, 6 Dec 2020 20:11:37 +0900 Subject: [PATCH 2/4] sd-device: use set_strjoin() This slightly changes TAGS= and CURRENT_TAGS= properties: Before E: TAGS=:aaa:bbb: After E: TAGS=aaa:bbb --- src/libsystemd/sd-device/sd-device.c | 63 +++++++--------------------- test/units/testsuite-55.sh | 44 ++++++++++++------- 2 files changed, 44 insertions(+), 63 deletions(-) diff --git a/src/libsystemd/sd-device/sd-device.c b/src/libsystemd/sd-device/sd-device.c index d06f90ce1d..005801e1a6 100644 --- a/src/libsystemd/sd-device/sd-device.c +++ b/src/libsystemd/sd-device/sd-device.c @@ -1518,30 +1518,6 @@ _public_ const char *sd_device_get_devlink_next(sd_device *device) { return v; } -static char *join_string_set(Set *s) { - size_t ret_allocated = 0, ret_len; - _cleanup_free_ char *ret = NULL; - const char *tag; - - if (!GREEDY_REALLOC(ret, ret_allocated, 2)) - return NULL; - - strcpy(ret, ":"); - ret_len = 1; - - SET_FOREACH(tag, s) { - char *e; - - if (!GREEDY_REALLOC(ret, ret_allocated, ret_len + strlen(tag) + 2)) - return NULL; - - e = stpcpy(stpcpy(ret + ret_len, tag), ":"); - ret_len = e - ret; - } - - return TAKE_PTR(ret); -} - int device_properties_prepare(sd_device *device) { int r; @@ -1557,46 +1533,39 @@ int device_properties_prepare(sd_device *device) { if (device->property_devlinks_outdated) { _cleanup_free_ char *devlinks = NULL; - size_t devlinks_allocated = 0, devlinks_len = 0; - const char *devlink; - for (devlink = sd_device_get_devlink_first(device); devlink; devlink = sd_device_get_devlink_next(device)) { - char *e; - - if (!GREEDY_REALLOC(devlinks, devlinks_allocated, devlinks_len + strlen(devlink) + 2)) - return -ENOMEM; - if (devlinks_len > 0) - stpcpy(devlinks + devlinks_len++, " "); - e = stpcpy(devlinks + devlinks_len, devlink); - devlinks_len = e - devlinks; - } - - r = device_add_property_internal(device, "DEVLINKS", devlinks); + r = set_strjoin(device->devlinks, " ", &devlinks); if (r < 0) return r; + if (!isempty(devlinks)) { + r = device_add_property_internal(device, "DEVLINKS", devlinks); + if (r < 0) + return r; + } + device->property_devlinks_outdated = false; } if (device->property_tags_outdated) { _cleanup_free_ char *tags = NULL; - tags = join_string_set(device->all_tags); - if (!tags) - return -ENOMEM; + r = set_strjoin(device->all_tags, ":", &tags); + if (r < 0) + return r; - if (!streq(tags, ":")) { + if (!isempty(tags)) { r = device_add_property_internal(device, "TAGS", tags); if (r < 0) return r; } - free(tags); - tags = join_string_set(device->current_tags); - if (!tags) - return -ENOMEM; + tags = mfree(tags); + r = set_strjoin(device->current_tags, ":", &tags); + if (r < 0) + return r; - if (!streq(tags, ":")) { + if (!isempty(tags)) { r = device_add_property_internal(device, "CURRENT_TAGS", tags); if (r < 0) return r; diff --git a/test/units/testsuite-55.sh b/test/units/testsuite-55.sh index ffceefb6a5..19f5683f57 100755 --- a/test/units/testsuite-55.sh +++ b/test/units/testsuite-55.sh @@ -2,14 +2,26 @@ set -ex set -o pipefail +function has_tag_internal() { + udevadm info /dev/null | sed -n '/E: '$1'=/ {s/E: '$1'=/:/; s/$/:/; p}' | grep -q ":$2:" +} + +function has_tag() { + has_tag_internal TAGS $1 +} + +function has_current_tag() { + has_tag_internal CURRENT_TAGS $1 +} + mkdir -p /run/udev/rules.d/ ! test -f /run/udev/tags/added/c1:3 && ! test -f /run/udev/tags/changed/c1:3 && - udevadm info /dev/null | grep -q -v 'E: TAGS=.*:added:.*' && - udevadm info /dev/null | grep -q -v 'E: CURRENT_TAGS=.*:added:.*' && - udevadm info /dev/null | grep -q -v 'E: TAGS=.*:changed:.*' && - udevadm info /dev/null | grep -q -v 'E: CURRENT_TAGS=.*:changed:.*' + ! has_tag added && + ! has_current_tag added && + ! has_tag changed && + ! has_current_tag changed cat > /run/udev/rules.d/50-testsuite.rules < Date: Sun, 6 Dec 2020 21:10:34 +0900 Subject: [PATCH 3/4] sd-device: keep escaped strings in DEVLINK= property This fixes a bug introduced by 87a4d416e5126b6fb2528ae192a6a6a8033539ce. Fixes #17772. --- src/libsystemd/sd-device/device-private.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/libsystemd/sd-device/device-private.c b/src/libsystemd/sd-device/device-private.c index 2801ebdcbe..9070dfbdd1 100644 --- a/src/libsystemd/sd-device/device-private.c +++ b/src/libsystemd/sd-device/device-private.c @@ -319,7 +319,10 @@ static int device_amend(sd_device *device, const char *key, const char *value) { for (const char *p = value;;) { _cleanup_free_ char *word = NULL; - r = extract_first_word(&p, &word, NULL, 0); + /* udev rules may set escaped strings, and sd-device does not modify the input + * strings. So, it is also necessary to keep the strings received through + * sd-device-monitor. */ + r = extract_first_word(&p, &word, NULL, EXTRACT_RETAIN_ESCAPE); if (r < 0) return r; if (r == 0) From e6f882871568e4a331ac473871ee8a884f6e48d6 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Sun, 6 Dec 2020 21:12:17 +0900 Subject: [PATCH 4/4] test: add tests for device_new_from_nulstr() --- src/libsystemd/sd-device/test-sd-device.c | 45 +++++++++++++++++++++++ 1 file changed, 45 insertions(+) diff --git a/src/libsystemd/sd-device/test-sd-device.c b/src/libsystemd/sd-device/test-sd-device.c index 9f48d2bf1e..fd30d1a1d0 100644 --- a/src/libsystemd/sd-device/test-sd-device.c +++ b/src/libsystemd/sd-device/test-sd-device.c @@ -1,9 +1,11 @@ /* SPDX-License-Identifier: LGPL-2.1-or-later */ #include "device-enumerator-private.h" +#include "device-internal.h" #include "device-private.h" #include "device-util.h" #include "hashmap.h" +#include "nulstr-util.h" #include "string-util.h" #include "tests.h" #include "time-util.h" @@ -161,6 +163,47 @@ static void test_sd_device_enumerator_filter_subsystem(void) { assert_se(n_new_dev <= 10); } +static void test_sd_device_new_from_nulstr(void) { + const char *devlinks = + "/dev/disk/by-partuuid/1290d63a-42cc-4c71-b87c-xxxxxxxxxxxx\0" + "/dev/disk/by-path/pci-0000:00:0f.0-scsi-0:0:0:0-part3\0" + "/dev/disk/by-label/Arch\\x20Linux\0" + "/dev/disk/by-uuid/a07b87e5-4af5-4a59-bde9-yyyyyyyyyyyy\0" + "/dev/disk/by-partlabel/Arch\\x20Linux\0" + "\0"; + + _cleanup_(sd_device_unrefp) sd_device *device = NULL, *from_nulstr = NULL; + _cleanup_free_ uint8_t *nulstr_copy = NULL; + const char *devlink; + const uint8_t *nulstr; + size_t len; + + log_info("/* %s */", __func__); + + assert_se(sd_device_new_from_syspath(&device, "/sys/class/net/lo") >= 0); + + /* Yeah, of course, setting devlink to the loopback interface is nonsense. But this is just a + * test for generating and parsing nulstr. For issue #17772. */ + NULSTR_FOREACH(devlink, devlinks) { + log_device_info(device, "setting devlink: %s", devlink); + assert_se(device_add_devlink(device, devlink) >= 0); + assert_se(set_contains(device->devlinks, devlink)); + } + + /* These properties are necessary for device_new_from_nulstr(). See device_verify(). */ + assert_se(device_add_property_internal(device, "SEQNUM", "1") >= 0); + assert_se(device_add_property_internal(device, "ACTION", "change") >= 0); + + assert_se(device_get_properties_nulstr(device, &nulstr, &len) >= 0); + assert_se(nulstr_copy = newdup(uint8_t, nulstr, len)); + assert_se(device_new_from_nulstr(&from_nulstr, nulstr_copy, len) >= 0); + + NULSTR_FOREACH(devlink, devlinks) { + log_device_info(from_nulstr, "checking devlink: %s", devlink); + assert_se(set_contains(from_nulstr->devlinks, devlink)); + } +} + int main(int argc, char **argv) { test_setup_logging(LOG_INFO); @@ -168,5 +211,7 @@ int main(int argc, char **argv) { test_sd_device_enumerator_subsystems(); test_sd_device_enumerator_filter_subsystem(); + test_sd_device_new_from_nulstr(); + return 0; }