path-util: rework file_in_same_dir() on top of path_extract_directory()

Let's port one more over.

Note that this changes behaviour of file_in_same_dir() in some regards.
Specifically, a trailing slash of the input path will be treated
differently: previously we'd operate below that dir then, instead of the
parent. I think that makes little sense however, and I think the code
using this function doesn't expect that either.

Moroever, addresses some corner cases if the path is specified as "/" or
".", i.e. where e cannot extract a parent. These will now be treated as
error, which I think is much cleaner.

diff --git a/src/basic/fs-util.c b/src/basic/fs-util.c
index ed020b1b08b988369d95aa01f3f358d6b34958e2..a895f4f2df2eb3c851d90704b6afab8acc4d6d8b 100644 (file)
--- a/src/basic/fs-util.c
+++ b/src/basic/fs-util.c
@@ -174,24 +174,18 @@ int readlink_value(const char *p, char **ret) {
         return 0;
 }
 
-int readlink_and_make_absolute(const char *p, char **r) {
+int readlink_and_make_absolute(const char *p, char **ret) {
         _cleanup_free_ char *target = NULL;
-        char *k;
-        int j;
+        int r;
 
         assert(p);
-        assert(r);
-
-        j = readlink_malloc(p, &target);
-        if (j < 0)
-                return j;
+        assert(ret);
 
-        k = file_in_same_dir(p, target);
-        if (!k)
-                return -ENOMEM;
+        r = readlink_malloc(p, &target);
+        if (r < 0)
+                return r;
 
-        *r = k;
-        return 0;
+        return file_in_same_dir(p, target, ret);
 }
 
 int chmod_and_chown_at(int dir_fd, const char *path, mode_t mode, uid_t uid, gid_t gid) {

diff --git a/src/basic/path-util.c b/src/basic/path-util.c
index cc45cb311e778f528b9a6a5314d531c52b51fa47..b442eebb8776065a245cfc5b6a0cc97fe5685ecc 100644 (file)
--- a/src/basic/path-util.c
+++ b/src/basic/path-util.c
@@ -1164,31 +1164,35 @@ bool path_is_normalized(const char *p) {
         return true;
 }
 
-char *file_in_same_dir(const char *path, const char *filename) {
-        char *e, *ret;
-        size_t k;
+int file_in_same_dir(const char *path, const char *filename, char **ret) {
+        _cleanup_free_ char *b = NULL;
+        int r;
 
         assert(path);
         assert(filename);
+        assert(ret);
 
-        /* This removes the last component of path and appends
-         * filename, unless the latter is absolute anyway or the
-         * former isn't */
+        /* This removes the last component of path and appends filename, unless the latter is absolute anyway
+         * or the former isn't */
 
         if (path_is_absolute(filename))
-                return strdup(filename);
-
-        e = strrchr(path, '/');
-        if (!e)
-                return strdup(filename);
+                b = strdup(filename);
+        else {
+                _cleanup_free_ char *dn = NULL;
 
-        k = strlen(filename);
-        ret = new(char, (e + 1 - path) + k + 1);
-        if (!ret)
-                return NULL;
+                r = path_extract_directory(path, &dn);
+                if (r == -EDESTADDRREQ) /* no path prefix */
+                        b = strdup(filename);
+                else if (r < 0)
+                        return r;
+                else
+                        b = path_join(dn, filename);
+        }
+        if (!b)
+                return -ENOMEM;
 
-        memcpy(mempcpy(ret, path, e + 1 - path), filename, k + 1);
-        return ret;
+        *ret = TAKE_PTR(b);
+        return 0;
 }
 
 bool hidden_or_backup_file(const char *filename) {

diff --git a/src/basic/path-util.h b/src/basic/path-util.h
index e40899284c772cf5ab0c7bdb4ed10a971fcaad00..56f01f41d8dfc409e16b67907d86faad0f8ba68c 100644 (file)
--- a/src/basic/path-util.h
+++ b/src/basic/path-util.h
@@ -170,7 +170,7 @@ static inline bool path_is_safe(const char *p) {
 }
 bool path_is_normalized(const char *p) _pure_;
 
-char *file_in_same_dir(const char *path, const char *filename);
+int file_in_same_dir(const char *path, const char *filename, char **ret);
 
 bool hidden_or_backup_file(const char *filename) _pure_;
 

diff --git a/src/nspawn/nspawn.c b/src/nspawn/nspawn.c
index 9f747a72b1968c94d5011297cd2cb767722f2b00..25f77509124b70bda1836575fdfd6970c601397a 100644 (file)
--- a/src/nspawn/nspawn.c
+++ b/src/nspawn/nspawn.c
@@ -4681,13 +4681,13 @@ static int load_settings(void) {
                  * actual image we shall boot. */
 
                 if (arg_image) {
-                        p = file_in_same_dir(arg_image, arg_settings_filename);
-                        if (!p)
-                                return log_oom();
-                } else if (arg_directory && !path_equal(arg_directory, "/")) {
-                        p = file_in_same_dir(arg_directory, arg_settings_filename);
-                        if (!p)
-                                return log_oom();
+                        r = file_in_same_dir(arg_image, arg_settings_filename, &p);
+                        if (r < 0)
+                                return log_error_errno(r, "Failed to generate settings path from image path: %m");
+                } else if (arg_directory) {
+                        r = file_in_same_dir(arg_directory, arg_settings_filename, &p);
+                        if (r < 0 && r != -EADDRNOTAVAIL) /* if directory is root fs, don't complain */
+                                return log_error_errno(r, "Failed to generate settings path from directory path: %m");
                 }
 
                 if (p) {

diff --git a/src/shared/discover-image.c b/src/shared/discover-image.c
index a7d5267696f9ef0d19872a2d1cb8ed46dd131176..6d294efd848ecff315e8228564148d2106ec2221 100644 (file)
--- a/src/shared/discover-image.c
+++ b/src/shared/discover-image.c
@@ -85,8 +85,9 @@ DEFINE_HASH_OPS_WITH_VALUE_DESTRUCTOR(image_hash_ops, char, string_hash_func, st
 
 static char **image_settings_path(Image *image) {
         _cleanup_strv_free_ char **l = NULL;
-        const char *fn;
-        unsigned i = 0;
+        _cleanup_free_ char *fn = NULL;
+        size_t i = 0;
+        int r;
 
         assert(image);
 
@@ -94,7 +95,9 @@ static char **image_settings_path(Image *image) {
         if (!l)
                 return NULL;
 
-        fn = strjoina(image->name, ".nspawn");
+        fn = strjoin(image->name, ".nspawn");
+        if (!fn)
+                return NULL;
 
         FOREACH_STRING(s, "/etc/systemd/nspawn", "/run/systemd/nspawn") {
                 l[i] = path_join(s, fn);
@@ -104,21 +107,27 @@ static char **image_settings_path(Image *image) {
                 i++;
         }
 
-        l[i] = file_in_same_dir(image->path, fn);
-        if (!l[i])
+        r = file_in_same_dir(image->path, fn, l + i);
+        if (r == -ENOMEM)
                 return NULL;
+        if (r < 0)
+                log_debug_errno(r, "Failed to generate .nspawn settings path from image path, ignoring: %m");
+
+        strv_uniq(l);
 
         return TAKE_PTR(l);
 }
 
-static char *image_roothash_path(Image *image) {
-        const char *fn;
+static int image_roothash_path(Image *image, char **ret) {
+        _cleanup_free_ char *fn = NULL;
 
         assert(image);
 
-        fn = strjoina(image->name, ".roothash");
+        fn = strjoin(image->name, ".roothash");
+        if (!fn)
+                return -ENOMEM;
 
-        return file_in_same_dir(image->path, fn);
+        return file_in_same_dir(image->path, fn, ret);
 }
 
 static int image_new(
@@ -646,9 +655,9 @@ int image_remove(Image *i) {
         if (!settings)
                 return -ENOMEM;
 
-        roothash = image_roothash_path(i);
-        if (!roothash)
-                return -ENOMEM;
+        r = image_roothash_path(i, &roothash);
+        if (r < 0)
+                return r;
 
         /* Make sure we don't interfere with a running nspawn */
         r = image_path_lock(i->path, LOCK_EX|LOCK_NB, &global_lock, &local_lock);
@@ -708,15 +717,17 @@ int image_remove(Image *i) {
 }
 
 static int rename_auxiliary_file(const char *path, const char *new_name, const char *suffix) {
-        _cleanup_free_ char *rs = NULL;
-        const char *fn;
-
-        fn = strjoina(new_name, suffix);
+        _cleanup_free_ char *fn = NULL, *rs = NULL;
+        int r;
 
-        rs = file_in_same_dir(path, fn);
-        if (!rs)
+        fn = strjoin(new_name, suffix);
+        if (!fn)
                 return -ENOMEM;
 
+        r = file_in_same_dir(path, fn, &rs);
+        if (r < 0)
+                return r;
+
         return rename_noreplace(AT_FDCWD, path, AT_FDCWD, rs);
 }
 
@@ -739,9 +750,9 @@ int image_rename(Image *i, const char *new_name) {
         if (!settings)
                 return -ENOMEM;
 
-        roothash = image_roothash_path(i);
-        if (!roothash)
-                return -ENOMEM;
+        r = image_roothash_path(i, &roothash);
+        if (r < 0)
+                return r;
 
         /* Make sure we don't interfere with a running nspawn */
         r = image_path_lock(i->path, LOCK_EX|LOCK_NB, &global_lock, &local_lock);
@@ -772,7 +783,7 @@ int image_rename(Image *i, const char *new_name) {
 
                 _fallthrough_;
         case IMAGE_SUBVOLUME:
-                new_path = file_in_same_dir(i->path, new_name);
+                r = file_in_same_dir(i->path, new_name, &new_path);
                 break;
 
         case IMAGE_BLOCK:
@@ -781,23 +792,23 @@ int image_rename(Image *i, const char *new_name) {
                 if (path_startswith(i->path, "/dev"))
                         return -EROFS;
 
-                new_path = file_in_same_dir(i->path, new_name);
+                r = file_in_same_dir(i->path, new_name, &new_path);
                 break;
 
         case IMAGE_RAW: {
                 const char *fn;
 
                 fn = strjoina(new_name, ".raw");
-                new_path = file_in_same_dir(i->path, fn);
+
+                r = file_in_same_dir(i->path, fn, &new_path);
                 break;
         }
 
         default:
                 return -EOPNOTSUPP;
         }
-
-        if (!new_path)
-                return -ENOMEM;
+        if (r < 0)
+                return r;
 
         nn = strdup(new_name);
         if (!nn)
@@ -828,15 +839,17 @@ int image_rename(Image *i, const char *new_name) {
 }
 
 static int clone_auxiliary_file(const char *path, const char *new_name, const char *suffix) {
-        _cleanup_free_ char *rs = NULL;
-        const char *fn;
-
-        fn = strjoina(new_name, suffix);
+        _cleanup_free_ char *fn = NULL, *rs = NULL;
+        int r;
 
-        rs = file_in_same_dir(path, fn);
-        if (!rs)
+        fn = strjoin(new_name, suffix);
+        if (!fn)
                 return -ENOMEM;
 
+        r = file_in_same_dir(path, fn, &rs);
+        if (r < 0)
+                return r;
+
         return copy_file_atomic(path, rs, 0664, 0, 0, COPY_REFLINK);
 }
 
@@ -856,9 +869,9 @@ int image_clone(Image *i, const char *new_name, bool read_only) {
         if (!settings)
                 return -ENOMEM;
 
-        roothash = image_roothash_path(i);
-        if (!roothash)
-                return -ENOMEM;
+        r = image_roothash_path(i, &roothash);
+        if (r < 0)
+                return r;
 
         /* Make sure nobody takes the new name, between the time we
          * checked it is currently unused in all search paths, and the

diff --git a/src/test/test-path-util.c b/src/test/test-path-util.c
index 3669dcbb1230d2f2afd1b5a6e9ff2bfd9482f5c9..136005d51fe7f1cf4f053f5e7a8ff82cadcdaf1f 100644 (file)
--- a/src/test/test-path-util.c
+++ b/src/test/test-path-util.c
@@ -600,23 +600,19 @@ TEST(prefix_root) {
 TEST(file_in_same_dir) {
         char *t;
 
-        t = file_in_same_dir("/", "a");
-        assert_se(streq(t, "/a"));
-        free(t);
+        assert_se(file_in_same_dir("/", "a", &t) == -EADDRNOTAVAIL);
 
-        t = file_in_same_dir("/", "/a");
+        assert_se(file_in_same_dir("/", "/a", &t) >= 0);
         assert_se(streq(t, "/a"));
         free(t);
 
-        t = file_in_same_dir("", "a");
-        assert_se(streq(t, "a"));
-        free(t);
+        assert_se(file_in_same_dir("", "a", &t) == -EINVAL);
 
-        t = file_in_same_dir("a/", "a");
-        assert_se(streq(t, "a/a"));
+        assert_se(file_in_same_dir("a/", "x", &t) >= 0);
+        assert_se(streq(t, "x"));
         free(t);
 
-        t = file_in_same_dir("bar/foo", "bar");
+        assert_se(file_in_same_dir("bar/foo", "bar", &t) >= 0);
         assert_se(streq(t, "bar/bar"));
         free(t);
 }