From b559616f2321643c5194b474d39a722cefaf6059 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Sun, 9 Jul 2017 23:31:47 -0400 Subject: [PATCH 1/5] cryptsetup-generator: add a helper utility to create symlinks It seems that there's a common pattern among the various generators. Let's add a helper function for it and make use of it in cryptsetup-generator. This fixes a bunch of theoretical memleaks in error paths, since *to wasn't generally freed properly. Not thath it matters. --- src/cryptsetup/cryptsetup-generator.c | 51 ++++++++------------------- src/shared/generator.c | 15 ++++++++ src/shared/generator.h | 2 ++ 3 files changed, 31 insertions(+), 37 deletions(-) diff --git a/src/cryptsetup/cryptsetup-generator.c b/src/cryptsetup/cryptsetup-generator.c index b58b6db7c9..3fdf65be65 100644 --- a/src/cryptsetup/cryptsetup-generator.c +++ b/src/cryptsetup/cryptsetup-generator.c @@ -58,11 +58,11 @@ static int create_disk( const char *password, const char *options) { - _cleanup_free_ char *p = NULL, *n = NULL, *d = NULL, *u = NULL, *to = NULL, *e = NULL, + _cleanup_free_ char *p = NULL, *n = NULL, *d = NULL, *u = NULL, *e = NULL, *filtered = NULL; _cleanup_fclose_ FILE *f = NULL; + const char *dmname; bool noauto, nofail, tmp, swap; - char *from; int r; assert(name); @@ -121,7 +121,7 @@ static int create_disk( if (password) { if (STR_IN_SET(password, "/dev/urandom", "/dev/random", "/dev/hw_random")) fputs("After=systemd-random-seed.service\n", f); - else if (!streq(password, "-") && !streq(password, "none")) { + else if (!STR_IN_SET(password, "-", "none")) { _cleanup_free_ char *uu; uu = fstab_node_to_udev_node(password); @@ -187,46 +187,23 @@ static int create_disk( if (r < 0) return log_error_errno(r, "Failed to write file %s: %m", p); - from = strjoina("../", n); - if (!noauto) { + r = generator_add_symlink(arg_dest, d, "wants", n); + if (r < 0) + return r; - to = strjoin(arg_dest, "/", d, ".wants/", n); - if (!to) - return log_oom(); - - mkdir_parents_label(to, 0755); - if (symlink(from, to) < 0) - return log_error_errno(errno, "Failed to create symlink %s: %m", to); - - free(to); - if (!nofail) - to = strjoin(arg_dest, "/cryptsetup.target.requires/", n); - else - to = strjoin(arg_dest, "/cryptsetup.target.wants/", n); - if (!to) - return log_oom(); - - mkdir_parents_label(to, 0755); - if (symlink(from, to) < 0) - return log_error_errno(errno, "Failed to create symlink %s: %m", to); + r = generator_add_symlink(arg_dest, "cryptsetup.target", + nofail ? "wants" : "requires", n); + if (r < 0) + return r; } - free(to); - to = strjoin(arg_dest, "/dev-mapper-", e, ".device.requires/", n); - if (!to) - return log_oom(); - - mkdir_parents_label(to, 0755); - if (symlink(from, to) < 0) - return log_error_errno(errno, "Failed to create symlink %s: %m", to); + dmname = strjoina("dev-mapper-", e, ".device"); + r = generator_add_symlink(arg_dest, dmname, "requires", n); + if (r < 0) + return r; if (!noauto && !nofail) { - _cleanup_free_ char *dmname; - dmname = strjoin("dev-mapper-", e, ".device"); - if (!dmname) - return log_oom(); - r = write_drop_in(arg_dest, dmname, 90, "device-timeout", "# Automatically generated by systemd-cryptsetup-generator \n\n" "[Unit]\nJobTimeoutSec=0"); diff --git a/src/shared/generator.c b/src/shared/generator.c index 6a78ebbda7..0a829dc9af 100644 --- a/src/shared/generator.c +++ b/src/shared/generator.c @@ -37,6 +37,21 @@ #include "unit-name.h" #include "util.h" +int generator_add_symlink(const char *root, const char *dst, const char *dep_type, const char *src) { + /* Adds a symlink from ..d/ to ../ */ + + const char *from, *to; + + from = strjoina("../", src); + to = strjoina(root, "/", dst, ".", dep_type, "/", src); + + mkdir_parents_label(to, 0755); + if (symlink(from, to) < 0) + return log_error_errno(errno, "Failed to create symlink \"%s\": %m", to); + + return 0; +} + static int write_fsck_sysroot_service(const char *dir, const char *what) { _cleanup_free_ char *device = NULL, *escaped = NULL; _cleanup_fclose_ FILE *f = NULL; diff --git a/src/shared/generator.h b/src/shared/generator.h index 825d934c8e..e70016839f 100644 --- a/src/shared/generator.h +++ b/src/shared/generator.h @@ -21,6 +21,8 @@ #include +int generator_add_symlink(const char *root, const char *dst, const char *dep_type, const char *src); + int generator_write_fsck_deps( FILE *f, const char *dir, From 630d30d37045d07db0ebe6c0b2729333f872e73a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Sun, 9 Jul 2017 23:40:14 -0400 Subject: [PATCH 2/5] fstab-generator: use generator_add_symlink() --- src/fstab-generator/fstab-generator.c | 39 ++++++++++----------------- 1 file changed, 14 insertions(+), 25 deletions(-) diff --git a/src/fstab-generator/fstab-generator.c b/src/fstab-generator/fstab-generator.c index ce9dccfe68..f72ca99535 100644 --- a/src/fstab-generator/fstab-generator.c +++ b/src/fstab-generator/fstab-generator.c @@ -91,7 +91,7 @@ static int add_swap( bool noauto, bool nofail) { - _cleanup_free_ char *name = NULL, *unit = NULL, *lnk = NULL; + _cleanup_free_ char *name = NULL, *unit = NULL; _cleanup_fclose_ FILE *f = NULL; int r; @@ -148,14 +148,10 @@ static int add_swap( return r; if (!noauto) { - lnk = strjoin(arg_dest, "/" SPECIAL_SWAP_TARGET, - nofail ? ".wants/" : ".requires/", name, NULL); - if (!lnk) - return log_oom(); - - mkdir_parents_label(lnk, 0755); - if (symlink(unit, lnk) < 0) - return log_error_errno(errno, "Failed to create symlink %s: %m", lnk); + r = generator_add_symlink(arg_dest, SPECIAL_SWAP_TARGET, + nofail ? "wants" : "requires", name); + if (r < 0) + return r; } return 0; @@ -300,7 +296,7 @@ static int add_mount( const char *source) { _cleanup_free_ char - *name = NULL, *unit = NULL, *lnk = NULL, + *name = NULL, *unit = NULL, *automount_name = NULL, *automount_unit = NULL, *filtered = NULL; _cleanup_fclose_ FILE *f = NULL; @@ -430,13 +426,10 @@ static int add_mount( return log_error_errno(r, "Failed to write unit file %s: %m", unit); if (!noauto && !automount) { - lnk = strjoin(dest, "/", post, nofail ? ".wants/" : ".requires/", name); - if (!lnk) - return log_oom(); - - mkdir_parents_label(lnk, 0755); - if (symlink(unit, lnk) < 0) - return log_error_errno(errno, "Failed to create symlink %s: %m", lnk); + r = generator_add_symlink(dest, post, + nofail ? "wants" : "requires", name); + if (r < 0) + return r; } if (automount) { @@ -491,14 +484,10 @@ static int add_mount( if (r < 0) return log_error_errno(r, "Failed to write unit file %s: %m", automount_unit); - free(lnk); - lnk = strjoin(dest, "/", post, nofail ? ".wants/" : ".requires/", automount_name); - if (!lnk) - return log_oom(); - - mkdir_parents_label(lnk, 0755); - if (symlink(automount_unit, lnk) < 0) - return log_error_errno(errno, "Failed to create symlink %s: %m", lnk); + r = generator_add_symlink(dest, post, + nofail ? "wants" : "requires", automount_name); + if (r < 0) + return r; } return 0; From 9cdcf3681c1a4139aff34541b5648234bf4aa1b1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Sun, 9 Jul 2017 23:53:18 -0400 Subject: [PATCH 3/5] gpt-auto-generator: use generator_add_symlink() --- src/gpt-auto-generator/gpt-auto-generator.c | 76 ++++++--------------- 1 file changed, 19 insertions(+), 57 deletions(-) diff --git a/src/gpt-auto-generator/gpt-auto-generator.c b/src/gpt-auto-generator/gpt-auto-generator.c index a072242430..a6c0b33fbb 100644 --- a/src/gpt-auto-generator/gpt-auto-generator.c +++ b/src/gpt-auto-generator/gpt-auto-generator.c @@ -58,7 +58,7 @@ static bool arg_root_rw = false; static int add_cryptsetup(const char *id, const char *what, bool rw, bool require, char **device) { _cleanup_free_ char *e = NULL, *n = NULL, *p = NULL, *d = NULL, *to = NULL; _cleanup_fclose_ FILE *f = NULL; - char *from, *ret; + char *ret; int r; assert(id); @@ -109,35 +109,21 @@ static int add_cryptsetup(const char *id, const char *what, bool rw, bool requir if (r < 0) return log_error_errno(r, "Failed to write file %s: %m", p); - from = strjoina("../", n); - - to = strjoin(arg_dest, "/", d, ".wants/", n); - if (!to) - return log_oom(); - - mkdir_parents_label(to, 0755); - if (symlink(from, to) < 0) - return log_error_errno(errno, "Failed to create symlink %s: %m", to); + r = generator_add_symlink(arg_dest, d, "wants", n); + if (r < 0) + return r; if (require) { - free(to); + const char *dmname; - to = strjoin(arg_dest, "/cryptsetup.target.requires/", n); - if (!to) - return log_oom(); + r = generator_add_symlink(arg_dest, "cryptsetup.target", "requires", n); + if (r < 0) + return r; - mkdir_parents_label(to, 0755); - if (symlink(from, to) < 0) - return log_error_errno(errno, "Failed to create symlink %s: %m", to); - - free(to); - to = strjoin(arg_dest, "/dev-mapper-", e, ".device.requires/", n); - if (!to) - return log_oom(); - - mkdir_parents_label(to, 0755); - if (symlink(from, to) < 0) - return log_error_errno(errno, "Failed to create symlink %s: %m", to); + dmname = strjoina("dev-mapper-", e, ".device"); + r = generator_add_symlink(arg_dest, dmname, "requires", n); + if (r < 0) + return r; } free(p); @@ -173,7 +159,7 @@ static int add_mount( const char *description, const char *post) { - _cleanup_free_ char *unit = NULL, *lnk = NULL, *crypto_what = NULL, *p = NULL; + _cleanup_free_ char *unit = NULL, *crypto_what = NULL, *p = NULL; _cleanup_fclose_ FILE *f = NULL; int r; @@ -239,16 +225,8 @@ static int add_mount( if (r < 0) return log_error_errno(r, "Failed to write unit file %s: %m", p); - if (post) { - lnk = strjoin(arg_dest, "/", post, ".requires/", unit); - if (!lnk) - return log_oom(); - - mkdir_parents_label(lnk, 0755); - if (symlink(p, lnk) < 0) - return log_error_errno(errno, "Failed to create symlink %s: %m", lnk); - } - + if (post) + return generator_add_symlink(arg_dest, post, "requires", unit); return 0; } @@ -299,7 +277,7 @@ static int add_partition_mount( } static int add_swap(const char *path) { - _cleanup_free_ char *name = NULL, *unit = NULL, *lnk = NULL; + _cleanup_free_ char *name = NULL, *unit = NULL; _cleanup_fclose_ FILE *f = NULL; int r; @@ -341,15 +319,7 @@ static int add_swap(const char *path) { if (r < 0) return log_error_errno(r, "Failed to write unit file %s: %m", unit); - lnk = strjoin(arg_dest, "/" SPECIAL_SWAP_TARGET ".wants/", name); - if (!lnk) - return log_oom(); - - mkdir_parents_label(lnk, 0755); - if (symlink(unit, lnk) < 0) - return log_error_errno(errno, "Failed to create symlink %s: %m", lnk); - - return 0; + return generator_add_symlink(arg_dest, SPECIAL_SWAP_TARGET, "wants", name); } #ifdef ENABLE_EFI @@ -363,7 +333,7 @@ static int add_automount( const char *description, usec_t timeout) { - _cleanup_free_ char *unit = NULL, *lnk = NULL; + _cleanup_free_ char *unit = NULL; _cleanup_free_ char *opt, *p = NULL; _cleanup_fclose_ FILE *f = NULL; int r; @@ -418,15 +388,7 @@ static int add_automount( if (r < 0) return log_error_errno(r, "Failed to write unit file %s: %m", p); - lnk = strjoin(arg_dest, "/" SPECIAL_LOCAL_FS_TARGET ".wants/", unit); - if (!lnk) - return log_oom(); - mkdir_parents_label(lnk, 0755); - - if (symlink(p, lnk) < 0) - return log_error_errno(errno, "Failed to create symlink %s: %m", lnk); - - return 0; + return generator_add_symlink(arg_dest, SPECIAL_LOCAL_FS_TARGET, "wants", unit); } static int add_esp(DissectedPartition *p) { From 7f0cc63771aeb792bb5fff1e22c471132ffa054b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Sun, 9 Jul 2017 23:59:30 -0400 Subject: [PATCH 4/5] sysv-generator: use generator_add_symlink() generator_add_symlink() is extended to ignore EEXIST. This should be fine for all existing callers. There's a small difference in behaviour when adding symlinks in sysv-generator: the message is more generic and does not include ", ignored". But creation of symlinks shouldn't ever fail except if things are very wrong, so in practice this shouldn't matter. Test needed updating: os.path.exists(os.readlink(link)) only works if the link is absolute (or if we are in the right directory). Let's just use os.path.exists(link), which properly tests that the symlink target exists. --- src/shared/generator.c | 3 ++- src/sysv-generator/sysv-generator.c | 31 +++-------------------------- test/sysv-generator-test.py | 3 ++- 3 files changed, 7 insertions(+), 30 deletions(-) diff --git a/src/shared/generator.c b/src/shared/generator.c index 0a829dc9af..2f459bf778 100644 --- a/src/shared/generator.c +++ b/src/shared/generator.c @@ -47,7 +47,8 @@ int generator_add_symlink(const char *root, const char *dst, const char *dep_typ mkdir_parents_label(to, 0755); if (symlink(from, to) < 0) - return log_error_errno(errno, "Failed to create symlink \"%s\": %m", to); + if (errno != EEXIST) + return log_error_errno(errno, "Failed to create symlink \"%s\": %m", to); return 0; } diff --git a/src/sysv-generator/sysv-generator.c b/src/sysv-generator/sysv-generator.c index 9828078443..3f3237d9b3 100644 --- a/src/sysv-generator/sysv-generator.c +++ b/src/sysv-generator/sysv-generator.c @@ -28,6 +28,7 @@ #include "exit-status.h" #include "fd-util.h" #include "fileio.h" +#include "generator.h" #include "hashmap.h" #include "hexdecoct.h" #include "install.h" @@ -101,29 +102,6 @@ static void free_sysvstub_hashmapp(Hashmap **h) { hashmap_free(*h); } -static int add_symlink(const char *service, const char *where) { - const char *from, *to; - int r; - - assert(service); - assert(where); - - from = strjoina(arg_dest, "/", service); - to = strjoina(arg_dest, "/", where, ".wants/", service); - - mkdir_parents_label(to, 0755); - - r = symlink(from, to); - if (r < 0) { - if (errno == EEXIST) - return 0; - - return -errno; - } - - return 1; -} - static int add_alias(const char *service, const char *alias) { const char *link; int r; @@ -219,11 +197,8 @@ static int generate_unit_file(SysvStub *s) { if (r < 0) return log_error_errno(r, "Failed to write unit %s: %m", unit); - STRV_FOREACH(p, s->wanted_by) { - r = add_symlink(s->name, *p); - if (r < 0) - log_warning_errno(r, "Failed to create 'Wants' symlink to %s, ignoring: %m", *p); - } + STRV_FOREACH(p, s->wanted_by) + (void) generator_add_symlink(arg_dest, *p, "wants", s->name); return 1; } diff --git a/test/sysv-generator-test.py b/test/sysv-generator-test.py index d116fffe38..99e323db3b 100755 --- a/test/sysv-generator-test.py +++ b/test/sysv-generator-test.py @@ -153,7 +153,8 @@ class SysvGeneratorTest(unittest.TestCase): link = os.path.join(self.out_dir, '%s.target.wants' % target, unit) if target in targets: unit_file = os.readlink(link) - self.assertTrue(os.path.exists(unit_file)) + # os.path.exists() will fail on a dangling symlink + self.assertTrue(os.path.exists(link)) self.assertEqual(os.path.basename(unit_file), unit) else: self.assertFalse(os.path.exists(link), From 1256c4814f9ff0a1d31fcdf1463bcb25a77adf9e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Thu, 13 Jul 2017 19:21:40 -0400 Subject: [PATCH 5/5] meson: hook up sysv-generator-test --- test/meson.build | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/test/meson.build b/test/meson.build index 55e693ac72..a5d78fb87b 100644 --- a/test/meson.build +++ b/test/meson.build @@ -165,3 +165,9 @@ if install_tests install_dir : testsdir + '/testdata/' + subdir) endforeach endif + +############################################################ + +sysv_generator_test_py = find_program('sysv-generator-test.py') +test('sysv-generator-test', + sysv_generator_test_py)