From 3a6ce860ac3e3c549e92f5f3254d1934dc543c24 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Fri, 6 Apr 2018 18:53:57 +0200 Subject: [PATCH] machine-image: rework error handling 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 | 12 ++-- src/import/import.c | 14 ++-- src/import/pull.c | 14 ++-- src/machine/image-dbus.c | 4 +- src/machine/machined-dbus.c | 38 +++++----- src/nspawn/nspawn.c | 6 +- src/shared/machine-image.c | 138 ++++++++++++++++++++++++------------ 7 files changed, 138 insertions(+), 88 deletions(-) diff --git a/src/import/export.c b/src/import/export.c index b8bae74d1a..5ff219b033 100644 --- a/src/import/export.c +++ b/src/import/export.c @@ -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 diff --git a/src/import/import.c b/src/import/import.c index 04f404f832..5cef899226 100644 --- a/src/import/import.c +++ b/src/import/import.c @@ -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; } diff --git a/src/import/pull.c b/src/import/pull.c index 8a7eccdeda..dfbdda79f1 100644 --- a/src/import/pull.c +++ b/src/import/pull.c @@ -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; } diff --git a/src/machine/image-dbus.c b/src/machine/image-dbus.c index ae5aa30a90..d2f5dc4593 100644 --- a/src/machine/image-dbus.c +++ b/src/machine/image-dbus.c @@ -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; diff --git a/src/machine/machined-dbus.c b/src/machine/machined-dbus.c index 5cf0aa8838..0faefaf81f 100644 --- a/src/machine/machined-dbus.c +++ b/src/machine/machined-dbus.c @@ -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); diff --git a/src/nspawn/nspawn.c b/src/nspawn/nspawn.c index 9d89af2d24..be2f1715ad 100644 --- a/src/nspawn/nspawn.c +++ b/src/nspawn/nspawn.c @@ -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); diff --git a/src/shared/machine-image.c b/src/shared/machine-image.c index 6fabe4cf7e..0238c5c842 100644 --- a/src/shared/machine-image.c +++ b/src/shared/machine-image.c @@ -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) { -- 2.25.1