]> git.ipfire.org Git - thirdparty/systemd.git/commitdiff
machine-image: rework error handling
authorLennart Poettering <lennart@poettering.net>
Fri, 6 Apr 2018 16:53:57 +0000 (18:53 +0200)
committerLennart Poettering <lennart@poettering.net>
Thu, 24 May 2018 15:01:57 +0000 (17:01 +0200)
Let's rework error handling a bit in image_find() and friends: when we
can't find an image, return -ENOENT rather than 0. That's better as
before we violated the usual rule in our codebase that return parameters
are initialized when the return value is >= 0 and otherwise not touched.

This also makes enumeration and validation a bit more strict: we'll only
accept ".raw" as suffix for regular files, and filter out this suffix
handling on directories/subvolumes, where it makes no sense.

src/import/export.c
src/import/import.c
src/import/pull.c
src/machine/image-dbus.c
src/machine/machined-dbus.c
src/nspawn/nspawn.c
src/shared/machine-image.c

index b8bae74d1a39631b90d26d64333a322fd91efb80..5ff219b033535663839d8de5eb6f706d35087e55 100644 (file)
@@ -70,12 +70,10 @@ static int export_tar(int argc, char *argv[], void *userdata) {
 
         if (machine_name_is_valid(argv[1])) {
                 r = image_find(IMAGE_MACHINE, argv[1], &image);
+                if (r == -ENOENT)
+                        return log_error_errno(r, "Machine image %s not found.", argv[1]);
                 if (r < 0)
                         return log_error_errno(r, "Failed to look for machine %s: %m", argv[1]);
-                if (r == 0) {
-                        log_error("Machine image %s not found.", argv[1]);
-                        return -ENOENT;
-                }
 
                 local = image->path;
         } else
@@ -149,12 +147,10 @@ static int export_raw(int argc, char *argv[], void *userdata) {
 
         if (machine_name_is_valid(argv[1])) {
                 r = image_find(IMAGE_MACHINE, argv[1], &image);
+                if (r == -ENOENT)
+                        return log_error_errno(r, "Machine image %s not found.", argv[1]);
                 if (r < 0)
                         return log_error_errno(r, "Failed to look for machine %s: %m", argv[1]);
-                if (r == 0) {
-                        log_error("Machine image %s not found.", argv[1]);
-                        return -ENOENT;
-                }
 
                 local = image->path;
         } else
index 04f404f832ca8bf695390c4048efd30ea5397c7c..5cef899226b18398a25a4d7cd5babac4df2677ec 100644 (file)
@@ -76,9 +76,10 @@ static int import_tar(int argc, char *argv[], void *userdata) {
 
                 if (!arg_force) {
                         r = image_find(IMAGE_MACHINE, local, NULL);
-                        if (r < 0)
-                                return log_error_errno(r, "Failed to check whether image '%s' exists: %m", local);
-                        else if (r > 0) {
+                        if (r < 0) {
+                                if (r != -ENOENT)
+                                        return log_error_errno(r, "Failed to check whether image '%s' exists: %m", local);
+                        } else {
                                 log_error("Image '%s' already exists.", local);
                                 return -EEXIST;
                         }
@@ -171,9 +172,10 @@ static int import_raw(int argc, char *argv[], void *userdata) {
 
                 if (!arg_force) {
                         r = image_find(IMAGE_MACHINE, local, NULL);
-                        if (r < 0)
-                                return log_error_errno(r, "Failed to check whether image '%s' exists: %m", local);
-                        else if (r > 0) {
+                        if (r < 0) {
+                                if (r != -ENOENT)
+                                        return log_error_errno(r, "Failed to check whether image '%s' exists: %m", local);
+                        } else {
                                 log_error("Image '%s' already exists.", local);
                                 return -EEXIST;
                         }
index 8a7eccdeda2541988cafeff7a0c76bbbec108840..dfbdda79f17f7ae341b72e014f44c348fba170ea 100644 (file)
@@ -84,9 +84,10 @@ static int pull_tar(int argc, char *argv[], void *userdata) {
 
                 if (!arg_force) {
                         r = image_find(IMAGE_MACHINE, local, NULL);
-                        if (r < 0)
-                                return log_error_errno(r, "Failed to check whether image '%s' exists: %m", local);
-                        else if (r > 0) {
+                        if (r < 0) {
+                                if (r != -ENOENT)
+                                        return log_error_errno(r, "Failed to check whether image '%s' exists: %m", local);
+                        } else {
                                 log_error("Image '%s' already exists.", local);
                                 return -EEXIST;
                         }
@@ -170,9 +171,10 @@ static int pull_raw(int argc, char *argv[], void *userdata) {
 
                 if (!arg_force) {
                         r = image_find(IMAGE_MACHINE, local, NULL);
-                        if (r < 0)
-                                return log_error_errno(r, "Failed to check whether image '%s' exists: %m", local);
-                        else if (r > 0) {
+                        if (r < 0) {
+                                if (r != -ENOENT)
+                                        return log_error_errno(r, "Failed to check whether image '%s' exists: %m", local);
+                        } else {
                                 log_error("Image '%s' already exists.", local);
                                 return -EEXIST;
                         }
index ae5aa30a90e30296167cd6203dde02472d6b48f2..d2f5dc459368035b878055816aa563c611786a8a 100644 (file)
@@ -436,7 +436,9 @@ int image_object_find(sd_bus *bus, const char *path, const char *interface, void
                 return r;
 
         r = image_find(IMAGE_MACHINE, e, &image);
-        if (r <= 0)
+        if (r == -ENOENT)
+                return 0;
+        if (r < 0)
                 return r;
 
         image->userdata = m;
index 5cf0aa8838a570e7734acbdaf5601191416c63ca..0faefaf81fa7a24dcf6757f8ff5136f9e5ef8017 100644 (file)
@@ -146,7 +146,7 @@ static int method_get_image(sd_bus_message *message, void *userdata, sd_bus_erro
                 return r;
 
         r = image_find(IMAGE_MACHINE, name, NULL);
-        if (r == 0)
+        if (r == -ENOENT)
                 return sd_bus_error_setf(error, BUS_ERROR_NO_SUCH_IMAGE, "No image '%s' known", name);
         if (r < 0)
                 return r;
@@ -739,10 +739,10 @@ static int method_remove_image(sd_bus_message *message, void *userdata, sd_bus_e
                 return sd_bus_error_setf(error, SD_BUS_ERROR_INVALID_ARGS, "Image name '%s' is invalid.", name);
 
         r = image_find(IMAGE_MACHINE, name, &i);
+        if (r == -ENOENT)
+                return sd_bus_error_setf(error, BUS_ERROR_NO_SUCH_IMAGE, "No image '%s' known", name);
         if (r < 0)
                 return r;
-        if (r == 0)
-                return sd_bus_error_setf(error, BUS_ERROR_NO_SUCH_IMAGE, "No image '%s' known", name);
 
         i->userdata = userdata;
         return bus_image_method_remove(message, i, error);
@@ -763,10 +763,10 @@ static int method_rename_image(sd_bus_message *message, void *userdata, sd_bus_e
                 return sd_bus_error_setf(error, SD_BUS_ERROR_INVALID_ARGS, "Image name '%s' is invalid.", old_name);
 
         r = image_find(IMAGE_MACHINE, old_name, &i);
+        if (r == -ENOENT)
+                return sd_bus_error_setf(error, BUS_ERROR_NO_SUCH_IMAGE, "No image '%s' known", old_name);
         if (r < 0)
                 return r;
-        if (r == 0)
-                return sd_bus_error_setf(error, BUS_ERROR_NO_SUCH_IMAGE, "No image '%s' known", old_name);
 
         i->userdata = userdata;
         return bus_image_method_rename(message, i, error);
@@ -787,10 +787,10 @@ static int method_clone_image(sd_bus_message *message, void *userdata, sd_bus_er
                 return sd_bus_error_setf(error, SD_BUS_ERROR_INVALID_ARGS, "Image name '%s' is invalid.", old_name);
 
         r = image_find(IMAGE_MACHINE, old_name, &i);
+        if (r == -ENOENT)
+                return sd_bus_error_setf(error, BUS_ERROR_NO_SUCH_IMAGE, "No image '%s' known", old_name);
         if (r < 0)
                 return r;
-        if (r == 0)
-                return sd_bus_error_setf(error, BUS_ERROR_NO_SUCH_IMAGE, "No image '%s' known", old_name);
 
         i->userdata = userdata;
         return bus_image_method_clone(message, i, error);
@@ -811,10 +811,10 @@ static int method_mark_image_read_only(sd_bus_message *message, void *userdata,
                 return sd_bus_error_setf(error, SD_BUS_ERROR_INVALID_ARGS, "Image name '%s' is invalid.", name);
 
         r = image_find(IMAGE_MACHINE, name, &i);
+        if (r == -ENOENT)
+                return sd_bus_error_setf(error, BUS_ERROR_NO_SUCH_IMAGE, "No image '%s' known", name);
         if (r < 0)
                 return r;
-        if (r == 0)
-                return sd_bus_error_setf(error, BUS_ERROR_NO_SUCH_IMAGE, "No image '%s' known", name);
 
         i->userdata = userdata;
         return bus_image_method_mark_read_only(message, i, error);
@@ -835,10 +835,10 @@ static int method_get_image_hostname(sd_bus_message *message, void *userdata, sd
                 return sd_bus_error_setf(error, SD_BUS_ERROR_INVALID_ARGS, "Image name '%s' is invalid.", name);
 
         r = image_find(IMAGE_MACHINE, name, &i);
+        if (r == -ENOENT)
+                return sd_bus_error_setf(error, BUS_ERROR_NO_SUCH_IMAGE, "No image '%s' known", name);
         if (r < 0)
                 return r;
-        if (r == 0)
-                return sd_bus_error_setf(error, BUS_ERROR_NO_SUCH_IMAGE, "No image '%s' known", name);
 
         i->userdata = userdata;
         return bus_image_method_get_hostname(message, i, error);
@@ -859,10 +859,10 @@ static int method_get_image_machine_id(sd_bus_message *message, void *userdata,
                 return sd_bus_error_setf(error, SD_BUS_ERROR_INVALID_ARGS, "Image name '%s' is invalid.", name);
 
         r = image_find(IMAGE_MACHINE, name, &i);
+        if (r == -ENOENT)
+                return sd_bus_error_setf(error, BUS_ERROR_NO_SUCH_IMAGE, "No image '%s' known", name);
         if (r < 0)
                 return r;
-        if (r == 0)
-                return sd_bus_error_setf(error, BUS_ERROR_NO_SUCH_IMAGE, "No image '%s' known", name);
 
         i->userdata = userdata;
         return bus_image_method_get_machine_id(message, i, error);
@@ -883,10 +883,10 @@ static int method_get_image_machine_info(sd_bus_message *message, void *userdata
                 return sd_bus_error_setf(error, SD_BUS_ERROR_INVALID_ARGS, "Image name '%s' is invalid.", name);
 
         r = image_find(IMAGE_MACHINE, name, &i);
+        if (r == -ENOENT)
+                return sd_bus_error_setf(error, BUS_ERROR_NO_SUCH_IMAGE, "No image '%s' known", name);
         if (r < 0)
                 return r;
-        if (r == 0)
-                return sd_bus_error_setf(error, BUS_ERROR_NO_SUCH_IMAGE, "No image '%s' known", name);
 
         i->userdata = userdata;
         return bus_image_method_get_machine_info(message, i, error);
@@ -907,10 +907,10 @@ static int method_get_image_os_release(sd_bus_message *message, void *userdata,
                 return sd_bus_error_setf(error, SD_BUS_ERROR_INVALID_ARGS, "Image name '%s' is invalid.", name);
 
         r = image_find(IMAGE_MACHINE, name, &i);
+        if (r == -ENOENT)
+                return sd_bus_error_setf(error, BUS_ERROR_NO_SUCH_IMAGE, "No image '%s' known", name);
         if (r < 0)
                 return r;
-        if (r == 0)
-                return sd_bus_error_setf(error, BUS_ERROR_NO_SUCH_IMAGE, "No image '%s' known", name);
 
         i->userdata = userdata;
         return bus_image_method_get_os_release(message, i, error);
@@ -1212,10 +1212,10 @@ static int method_set_image_limit(sd_bus_message *message, void *userdata, sd_bu
                 return sd_bus_error_setf(error, SD_BUS_ERROR_INVALID_ARGS, "Image name '%s' is invalid.", name);
 
         r = image_find(IMAGE_MACHINE, name, &i);
+        if (r == -ENOENT)
+                return sd_bus_error_setf(error, BUS_ERROR_NO_SUCH_IMAGE, "No image '%s' known", name);
         if (r < 0)
                 return r;
-        if (r == 0)
-                return sd_bus_error_setf(error, BUS_ERROR_NO_SUCH_IMAGE, "No image '%s' known", name);
 
         i->userdata = userdata;
         return bus_image_method_set_limit(message, i, error);
index 9d89af2d24a63ef3122f019fc61b38ebd73a80d4..be2f1715ad1b5ccad93096ea8bcb2bc80eb7e173 100644 (file)
@@ -2400,12 +2400,10 @@ static int determine_names(void) {
                         _cleanup_(image_unrefp) Image *i = NULL;
 
                         r = image_find(IMAGE_MACHINE, arg_machine, &i);
+                        if (r == -ENOENT)
+                                return log_error_errno(r, "No image for machine '%s'.", arg_machine);
                         if (r < 0)
                                 return log_error_errno(r, "Failed to find image for machine '%s': %m", arg_machine);
-                        if (r == 0) {
-                                log_error("No image for machine '%s'.", arg_machine);
-                                return -ENOENT;
-                        }
 
                         if (IN_SET(i->type, IMAGE_RAW, IMAGE_BLOCK))
                                 r = free_and_strdup(&arg_image, i->path);
index 6fabe4cf7eb4efbfa9661e78c1adaad61ac140e0..0238c5c8425ffe2a7b3053f415e942d66aebde67 100644 (file)
@@ -146,7 +146,6 @@ static int image_new(
                 i->path = strjoin(path, "/", filename);
         else
                 i->path = strdup(filename);
-
         if (!i->path)
                 return -ENOMEM;
 
@@ -194,31 +193,40 @@ static int image_make(
                 int dfd,
                 const char *path,
                 const char *filename,
+                const struct stat *st,
                 Image **ret) {
 
         _cleanup_free_ char *pretty_buffer = NULL;
-        struct stat st;
+        struct stat stbuf;
         bool read_only;
         int r;
 
+        assert(dfd >= 0 || dfd == AT_FDCWD);
         assert(filename);
 
         /* We explicitly *do* follow symlinks here, since we want to allow symlinking trees, raw files and block
-         * devices into /var/lib/machines/, and treat them normally. */
+         * devices into /var/lib/machines/, and treat them normally.
+         *
+         * This function returns -ENOENT if we can't find the image after all, and -EMEDIUMTYPE if it's not a file we
+         * recognize. */
+
+        if (!st) {
+                if (fstatat(dfd, filename, &stbuf, 0) < 0)
+                        return -errno;
 
-        if (fstatat(dfd, filename, &st, 0) < 0)
-                return -errno;
+                st = &stbuf;
+        }
 
         read_only =
                 (path && path_startswith(path, "/usr")) ||
                 (faccessat(dfd, filename, W_OK, AT_EACCESS) < 0 && errno == EROFS);
 
-        if (S_ISDIR(st.st_mode)) {
+        if (S_ISDIR(st->st_mode)) {
                 _cleanup_close_ int fd = -1;
                 unsigned file_attr = 0;
 
                 if (!ret)
-                        return 1;
+                        return 0;
 
                 if (!pretty) {
                         r = extract_pretty(filename, NULL, &pretty_buffer);
@@ -233,7 +241,7 @@ static int image_make(
                         return -errno;
 
                 /* btrfs subvolumes have inode 256 */
-                if (st.st_ino == 256) {
+                if (st->st_ino == 256) {
 
                         r = btrfs_is_filesystem(fd);
                         if (r < 0)
@@ -271,7 +279,7 @@ static int image_make(
                                         }
                                 }
 
-                                return 1;
+                                return 0;
                         }
                 }
 
@@ -292,15 +300,15 @@ static int image_make(
                 if (r < 0)
                         return r;
 
-                return 1;
+                return 0;
 
-        } else if (S_ISREG(st.st_mode) && endswith(filename, ".raw")) {
+        } else if (S_ISREG(st->st_mode) && endswith(filename, ".raw")) {
                 usec_t crtime = 0;
 
                 /* It's a RAW disk image */
 
                 if (!ret)
-                        return 1;
+                        return 0;
 
                 (void) fd_getcrtime_at(dfd, filename, &crtime, 0);
 
@@ -316,26 +324,26 @@ static int image_make(
                               pretty,
                               path,
                               filename,
-                              !(st.st_mode & 0222) || read_only,
+                              !(st->st_mode & 0222) || read_only,
                               crtime,
-                              timespec_load(&st.st_mtim),
+                              timespec_load(&st->st_mtim),
                               ret);
                 if (r < 0)
                         return r;
 
-                (*ret)->usage = (*ret)->usage_exclusive = st.st_blocks * 512;
-                (*ret)->limit = (*ret)->limit_exclusive = st.st_size;
+                (*ret)->usage = (*ret)->usage_exclusive = st->st_blocks * 512;
+                (*ret)->limit = (*ret)->limit_exclusive = st->st_size;
 
-                return 1;
+                return 0;
 
-        } else if (S_ISBLK(st.st_mode)) {
+        } else if (S_ISBLK(st->st_mode)) {
                 _cleanup_close_ int block_fd = -1;
                 uint64_t size = UINT64_MAX;
 
                 /* A block device */
 
                 if (!ret)
-                        return 1;
+                        return 0;
 
                 if (!pretty) {
                         r = extract_pretty(filename, NULL, &pretty_buffer);
@@ -349,9 +357,12 @@ static int image_make(
                 if (block_fd < 0)
                         log_debug_errno(errno, "Failed to open block device %s/%s, ignoring: %m", path, filename);
                 else {
-                        if (fstat(block_fd, &st) < 0)
+                        /* Refresh stat data after opening the node */
+                        if (fstat(block_fd, &stbuf) < 0)
                                 return -errno;
-                        if (!S_ISBLK(st.st_mode)) /* Verify that what we opened is actually what we think it is */
+                        st = &stbuf;
+
+                        if (!S_ISBLK(st->st_mode)) /* Verify that what we opened is actually what we think it is */
                                 return -ENOTTY;
 
                         if (!read_only) {
@@ -373,7 +384,7 @@ static int image_make(
                               pretty,
                               path,
                               filename,
-                              !(st.st_mode & 0222) || read_only,
+                              !(st->st_mode & 0222) || read_only,
                               0,
                               0,
                               ret);
@@ -383,10 +394,10 @@ static int image_make(
                 if (size != 0 && size != UINT64_MAX)
                         (*ret)->usage = (*ret)->usage_exclusive = (*ret)->limit = (*ret)->limit_exclusive = size;
 
-                return 1;
+                return 0;
         }
 
-        return 0;
+        return -EMEDIUMTYPE;
 }
 
 int image_find(ImageClass class, const char *name, Image **ret) {
@@ -399,10 +410,11 @@ int image_find(ImageClass class, const char *name, Image **ret) {
 
         /* There are no images with invalid names */
         if (!image_name_is_valid(name))
-                return 0;
+                return -ENOENT;
 
         NULSTR_FOREACH(path, image_search_path[class]) {
                 _cleanup_closedir_ DIR *d = NULL;
+                struct stat st;
 
                 d = opendir(path);
                 if (!d) {
@@ -412,18 +424,39 @@ int image_find(ImageClass class, const char *name, Image **ret) {
                         return -errno;
                 }
 
-                r = image_make(name, dirfd(d), path, name, ret);
-                if (IN_SET(r, 0, -ENOENT)) {
+                /* As mentioned above, we follow symlinks on this fstatat(), because we want to permit people to
+                 * symlink block devices into the search path */
+                if (fstatat(dirfd(d), name, &st, 0) < 0) {
                         _cleanup_free_ char *raw = NULL;
 
+                        if (errno != ENOENT)
+                                return -errno;
+
                         raw = strappend(name, ".raw");
                         if (!raw)
                                 return -ENOMEM;
 
-                        r = image_make(name, dirfd(d), path, raw, ret);
-                        if (IN_SET(r, 0, -ENOENT))
+                        if (fstatat(dirfd(d), raw, &st, 0) < 0) {
+
+                                if (errno == ENOENT)
+                                        continue;
+
+                                return -errno;
+                        }
+
+                        if (!S_ISREG(st.st_mode))
                                 continue;
+
+                        r = image_make(name, dirfd(d), path, raw, &st, ret);
+
+                } else {
+                        if (!S_ISDIR(st.st_mode) && !S_ISBLK(st.st_mode))
+                                continue;
+
+                        r = image_make(name, dirfd(d), path, name, &st, ret);
                 }
+                if (IN_SET(r, -ENOENT, -EMEDIUMTYPE))
+                        continue;
                 if (r < 0)
                         return r;
 
@@ -431,9 +464,9 @@ int image_find(ImageClass class, const char *name, Image **ret) {
         }
 
         if (class == IMAGE_MACHINE && streq(name, ".host"))
-                return image_make(".host", AT_FDCWD, NULL, "/", ret);
+                return image_make(".host", AT_FDCWD, NULL, "/", NULL, ret);
 
-        return 0;
+        return -ENOENT;
 };
 
 int image_discover(ImageClass class, Hashmap *h) {
@@ -459,20 +492,37 @@ int image_discover(ImageClass class, Hashmap *h) {
                 FOREACH_DIRENT_ALL(de, d, return -errno) {
                         _cleanup_(image_unrefp) Image *image = NULL;
                         _cleanup_free_ char *truncated = NULL;
-                        const char *pretty, *e;
+                        const char *pretty;
+                        struct stat st;
 
                         if (dot_or_dot_dot(de->d_name))
                                 continue;
 
-                        e = endswith(de->d_name, ".raw");
-                        if (e) {
+                        /* As mentioned above, we follow symlinks on this fstatat(), because we want to permit people
+                         * to symlink block devices into the search path */
+                        if (fstatat(dirfd(d), de->d_name, &st, 0) < 0) {
+                                if (errno == ENOENT)
+                                        continue;
+
+                                return -errno;
+                        }
+
+                        if (S_ISREG(st.st_mode)) {
+                                const char *e;
+
+                                e = endswith(de->d_name, ".raw");
+                                if (!e)
+                                        continue;
+
                                 truncated = strndup(de->d_name, e - de->d_name);
                                 if (!truncated)
                                         return -ENOMEM;
 
                                 pretty = truncated;
-                        } else
+                        } else if (S_ISDIR(st.st_mode) || S_ISBLK(st.st_mode))
                                 pretty = de->d_name;
+                        else
+                                continue;
 
                         if (!image_name_is_valid(pretty))
                                 continue;
@@ -480,8 +530,8 @@ int image_discover(ImageClass class, Hashmap *h) {
                         if (hashmap_contains(h, pretty))
                                 continue;
 
-                        r = image_make(pretty, dirfd(d), path, de->d_name, &image);
-                        if (IN_SET(r, 0, -ENOENT))
+                        r = image_make(pretty, dirfd(d), path, de->d_name, &st, &image);
+                        if (IN_SET(r, -ENOENT, -EMEDIUMTYPE))
                                 continue;
                         if (r < 0)
                                 return r;
@@ -497,7 +547,7 @@ int image_discover(ImageClass class, Hashmap *h) {
         if (class == IMAGE_MACHINE && !hashmap_contains(h, ".host")) {
                 _cleanup_(image_unrefp) Image *image = NULL;
 
-                r = image_make(".host", AT_FDCWD, NULL, "/", &image);
+                r = image_make(".host", AT_FDCWD, NULL, "/", NULL, &image);
                 if (r < 0)
                         return r;
 
@@ -640,10 +690,10 @@ int image_rename(Image *i, const char *new_name) {
                 return r;
 
         r = image_find(IMAGE_MACHINE, new_name, NULL);
-        if (r < 0)
-                return r;
-        if (r > 0)
+        if (r >= 0)
                 return -EEXIST;
+        if (r != -ENOENT)
+                return r;
 
         switch (i->type) {
 
@@ -753,10 +803,10 @@ int image_clone(Image *i, const char *new_name, bool read_only) {
                 return r;
 
         r = image_find(IMAGE_MACHINE, new_name, NULL);
-        if (r < 0)
-                return r;
-        if (r > 0)
+        if (r >= 0)
                 return -EEXIST;
+        if (r != -ENOENT)
+                return r;
 
         switch (i->type) {