shared/kbd-util: fix return value confusion with nftw()

We would return a real error sometimes from the callback, and FTW_STOP other
times. Because of FTW_ACTIONRETVAL, everything except FTW_STOP would be
ignored. I don't think using FTW_ACTIONRETVAL is useful.

nftw() can only be used meaningfully with errno. Even if we return a proper
value ourselves from the callback, it will be propagated as a return value from
nftw(), but there is no way to distinguish this from a value generated by
nftw() itself, which is -1/-EPERM on error. So let's set errno ourselves so the
caller can at least look at that.

The code still ignores all errors.

diff --git a/TODO b/TODO
index 87a92af8061aa2a1b3b2f05ff23c59c6cb7c3b59..2e8f24750bc4eb85c67659315ab612ed88ef2be1 100644 (file)
--- a/TODO
+++ b/TODO
@@ -7,6 +7,8 @@ Bugfixes:
 * userdbctl: "Password OK: yes" is shown even when there are no passwords
   or the password is locked.
 
+* Get rid of nftw(). We should refuse to use such useless APIs on principle.
+
 External:
 
 * Fedora: add an rpmlint check that verifies that all unit files in the RPM are listed in %systemd_post macros.

diff --git a/src/shared/kbd-util.c b/src/shared/kbd-util.c
index 79ae96253032978de7a1e681d2ac4f5e629baa6a..fb98dd262e82b8fe1ca1c50ef675f6ca0556f4a2 100644 (file)
--- a/src/shared/kbd-util.c
+++ b/src/shared/kbd-util.c
@@ -31,14 +31,14 @@ static int nftw_cb(
                 return 0;
 
         p = strdup(basename(fpath));
-        if (!p)
-                return FTW_STOP;
+        if (!p) {
+                errno = ENOMEM;
+                return -1;
+        }
 
         e = endswith(p, ".map");
-        if (e)
-                *e = 0;
-
-        e = endswith(p, ".map.gz");
+        if (!e)
+                e = endswith(p, ".map.gz");
         if (e)
                 *e = 0;
 
@@ -46,37 +46,33 @@ static int nftw_cb(
                 return 0;
 
         r = set_consume(keymaps, TAKE_PTR(p));
-        if (r < 0 && r != -EEXIST)
-                return r;
+        if (r < 0 && r != -EEXIST) {
+                errno = -r;
+                return -1;
+        }
 
         return 0;
 }
 
 int get_keymaps(char ***ret) {
-        _cleanup_strv_free_ char **l = NULL;
-        const char *dir;
-        int r;
-
         keymaps = set_new(&string_hash_ops);
         if (!keymaps)
                 return -ENOMEM;
 
-        NULSTR_FOREACH(dir, KBD_KEYMAP_DIRS) {
-                r = nftw(dir, nftw_cb, 20, FTW_PHYS|FTW_ACTIONRETVAL);
-
-                if (r == FTW_STOP)
-                        log_debug("Directory not found %s", dir);
-                else if (r < 0)
-                        log_debug_errno(r, "Can't add keymap: %m");
-        }
+        const char *dir;
+        NULSTR_FOREACH(dir, KBD_KEYMAP_DIRS)
+                if (nftw(dir, nftw_cb, 20, FTW_PHYS) < 0) {
+                        if (errno != ENOENT)
+                                log_debug_errno(errno, "Failed to read keymap list from %s, ignoring: %m", dir);
+                }
 
-        l = set_get_strv(keymaps);
+        _cleanup_strv_free_ char **l = set_get_strv(keymaps);
         if (!l) {
-                set_free_free(keymaps);
+                keymaps = set_free_free(keymaps);
                 return -ENOMEM;
         }
 
-        set_free(keymaps);
+        keymaps = set_free(keymaps);
 
         if (strv_isempty(l))
                 return -ENOENT;