From 852c05c94fbe050f4db16b1919b717f882c7079e Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Wed, 22 Jan 2025 05:24:35 +0900 Subject: [PATCH] catalog: modernize code - set destructors to catalog_hash_ops, - acquire OrderedHashmap when necessary, - gracefully handle NULL catalog directories and output stream, - rename function output arguments, - add many many assertions, - use RET_GATHER(). --- src/fuzz/fuzz-catalog.c | 6 +- src/journal/journalctl-catalog.c | 6 +- src/libsystemd/sd-journal/catalog.c | 227 +++++++++++++---------- src/libsystemd/sd-journal/catalog.h | 15 +- src/libsystemd/sd-journal/test-catalog.c | 20 +- 5 files changed, 144 insertions(+), 130 deletions(-) diff --git a/src/fuzz/fuzz-catalog.c b/src/fuzz/fuzz-catalog.c index f9561f24c4e..cbe8463a89b 100644 --- a/src/fuzz/fuzz-catalog.c +++ b/src/fuzz/fuzz-catalog.c @@ -9,17 +9,15 @@ int LLVMFuzzerTestOneInput(const uint8_t *data, size_t size) { _cleanup_(unlink_tempfilep) char name[] = "/tmp/fuzz-catalog.XXXXXX"; _cleanup_close_ int fd = -EBADF; - _cleanup_ordered_hashmap_free_free_free_ OrderedHashmap *h = NULL; + _cleanup_ordered_hashmap_free_ OrderedHashmap *h = NULL; fuzz_setup_logging(); - assert_se(h = ordered_hashmap_new(&catalog_hash_ops)); - fd = mkostemp_safe(name); assert_se(fd >= 0); assert_se(write(fd, data, size) == (ssize_t) size); - (void) catalog_import_file(h, name); + (void) catalog_import_file(&h, name); return 0; } diff --git a/src/journal/journalctl-catalog.c b/src/journal/journalctl-catalog.c index 116e152be00..41ea3816e7b 100644 --- a/src/journal/journalctl-catalog.c +++ b/src/journal/journalctl-catalog.c @@ -19,7 +19,7 @@ int action_update_catalog(void) { e = secure_getenv("SYSTEMD_CATALOG_SOURCES"); r = catalog_update(database, arg_root, - e ? STRV_MAKE_CONST(e) : catalog_file_dirs); + e ? STRV_MAKE_CONST(e) : NULL); if (r < 0) return log_error_errno(r, "Failed to update catalog: %m"); @@ -41,9 +41,9 @@ int action_list_catalog(char **items) { pager_open(arg_pager_flags); if (items) - r = catalog_list_items(stdout, database, oneline, items); + r = catalog_list_items(/* f = */ NULL, database, oneline, items); else - r = catalog_list(stdout, database, oneline); + r = catalog_list(/* f = */ NULL, database, oneline); if (r < 0) return log_error_errno(r, "Failed to list catalog: %m"); diff --git a/src/libsystemd/sd-journal/catalog.c b/src/libsystemd/sd-journal/catalog.c index 7dcc35d8d58..4e3733ce2e9 100644 --- a/src/libsystemd/sd-journal/catalog.c +++ b/src/libsystemd/sd-journal/catalog.c @@ -55,15 +55,20 @@ typedef struct CatalogItem { } CatalogItem; static void catalog_hash_func(const CatalogItem *i, struct siphash *state) { + assert(i); + assert(state); + siphash24_compress_typesafe(i->id, state); siphash24_compress_string(i->language, state); } static int catalog_compare_func(const CatalogItem *a, const CatalogItem *b) { - unsigned k; int r; - for (k = 0; k < ELEMENTSOF(b->id.bytes); k++) { + assert(a); + assert(b); + + for (size_t k = 0; k < ELEMENTSOF(b->id.bytes); k++) { r = CMP(a->id.bytes[k], b->id.bytes[k]); if (r != 0) return r; @@ -72,11 +77,17 @@ static int catalog_compare_func(const CatalogItem *a, const CatalogItem *b) { return strcmp(a->language, b->language); } -DEFINE_HASH_OPS(catalog_hash_ops, CatalogItem, catalog_hash_func, catalog_compare_func); +DEFINE_PRIVATE_HASH_OPS_FULL( + catalog_hash_ops, + CatalogItem, catalog_hash_func, catalog_compare_func, free, + void, free); static bool next_header(const char **s) { const char *e; + assert(s); + assert(*s); + e = strchr(*s, '\n'); /* Unexpected end */ @@ -91,17 +102,22 @@ static bool next_header(const char **s) { return true; } -static const char *skip_header(const char *s) { +static const char* skip_header(const char *s) { + assert(s); + while (next_header(&s)) ; return s; } -static char *combine_entries(const char *one, const char *two) { +static char* combine_entries(const char *one, const char *two) { const char *b1, *b2; size_t l1, l2, n; char *dest, *p; + assert(one); + assert(two); + /* Find split point of headers to body */ b1 = skip_header(one); b2 = skip_header(two); @@ -140,10 +156,11 @@ static char *combine_entries(const char *one, const char *two) { } static int finish_item( - OrderedHashmap *h, + OrderedHashmap **h, sd_id128_t id, const char *language, - char *payload, size_t payload_size) { + const char *payload, + size_t payload_size) { _cleanup_free_ CatalogItem *i = NULL; _cleanup_free_ char *combined = NULL; @@ -164,14 +181,14 @@ static int finish_item( strcpy(i->language, language); } - prev = ordered_hashmap_get(h, i); + prev = ordered_hashmap_get(*h, i); if (prev) { /* Already have such an item, combine them */ combined = combine_entries(payload, prev); if (!combined) return log_oom(); - r = ordered_hashmap_update(h, i, combined); + r = ordered_hashmap_update(*h, i, combined); if (r < 0) return log_error_errno(r, "Failed to update catalog item: %m"); @@ -183,7 +200,7 @@ static int finish_item( if (!combined) return log_oom(); - r = ordered_hashmap_put(h, i, combined); + r = ordered_hashmap_ensure_put(h, &catalog_hash_ops, i, combined); if (r < 0) return log_error_errno(r, "Failed to insert catalog item: %m"); @@ -194,8 +211,11 @@ static int finish_item( return 0; } -int catalog_file_lang(const char* filename, char **lang) { - char *beg, *end, *_lang; +int catalog_file_lang(const char *filename, char **ret) { + char *beg, *end, *lang; + + assert(filename); + assert(ret); end = endswith(filename, ".catalog"); if (!end) @@ -208,21 +228,23 @@ int catalog_file_lang(const char* filename, char **lang) { if (*beg != '.' || end <= beg + 1) return 0; - _lang = strndup(beg + 1, end - beg - 1); - if (!_lang) + lang = strndup(beg + 1, end - beg - 1); + if (!lang) return -ENOMEM; - *lang = _lang; + *ret = lang; return 1; } static int catalog_entry_lang( - const char* filename, + const char *filename, unsigned line, - const char* t, - const char* deflang, + const char *t, + const char *deflang, char **ret) { + assert(t); + size_t c = strlen(t); if (c < 2) return log_error_errno(SYNTHETIC_ERRNO(EINVAL), @@ -243,7 +265,7 @@ static int catalog_entry_lang( return strdup_to(ret, t); } -int catalog_import_file(OrderedHashmap *h, const char *path) { +int catalog_import_file(OrderedHashmap **h, const char *path) { _cleanup_fclose_ FILE *f = NULL; _cleanup_free_ char *payload = NULL; size_t payload_size = 0; @@ -371,14 +393,19 @@ int catalog_import_file(OrderedHashmap *h, const char *path) { static int64_t write_catalog( const char *database, - struct strbuf *sb, - CatalogItem *items, - size_t n) { + const struct strbuf *sb, + const CatalogItem *items, + size_t n_items) { _cleanup_(unlink_and_freep) char *p = NULL; _cleanup_fclose_ FILE *w = NULL; int r; + assert(database); + assert(sb); + assert(items); + assert(n_items > 0); + r = mkdir_parents(database, 0755); if (r < 0) return log_error_errno(r, "Failed to create parent directories of %s: %m", database); @@ -391,13 +418,13 @@ static int64_t write_catalog( .signature = CATALOG_SIGNATURE, .header_size = htole64(CONST_ALIGN_TO(sizeof(CatalogHeader), 8)), .catalog_item_size = htole64(sizeof(CatalogItem)), - .n_items = htole64(n), + .n_items = htole64(n_items), }; if (fwrite(&header, sizeof(header), 1, w) != 1) return log_error_errno(SYNTHETIC_ERRNO(EIO), "%s: failed to write header.", p); - if (fwrite(items, sizeof(CatalogItem), n, w) != n) + if (fwrite(items, sizeof(CatalogItem), n_items, w) != n_items) return log_error_errno(SYNTHETIC_ERRNO(EIO), "%s: failed to write database.", p); if (fwrite(sb->buf, sb->len, 1, w) != 1) @@ -416,30 +443,23 @@ static int64_t write_catalog( return ftello(w); } -int catalog_update(const char* database, const char* root, const char* const* dirs) { - _cleanup_strv_free_ char **files = NULL; - _cleanup_(strbuf_freep) struct strbuf *sb = NULL; - _cleanup_ordered_hashmap_free_free_free_ OrderedHashmap *h = NULL; - _cleanup_free_ CatalogItem *items = NULL; - ssize_t offset; - char *payload; - CatalogItem *i; - unsigned n; +int catalog_update(const char *database, const char *root, const char* const *dirs) { int r; - int64_t sz; - h = ordered_hashmap_new(&catalog_hash_ops); - sb = strbuf_new(); - if (!h || !sb) - return log_oom(); + assert(database); + + if (!dirs) + dirs = catalog_file_dirs; + _cleanup_strv_free_ char **files = NULL; r = conf_files_list_strv(&files, ".catalog", root, 0, dirs); if (r < 0) return log_error_errno(r, "Failed to get catalog files: %m"); + _cleanup_ordered_hashmap_free_ OrderedHashmap *h = NULL; STRV_FOREACH(f, files) { log_debug("Reading file '%s'", *f); - r = catalog_import_file(h, *f); + r = catalog_import_file(&h, *f); if (r < 0) return log_error_errno(r, "Failed to import file '%s': %m", *f); } @@ -451,17 +471,23 @@ int catalog_update(const char* database, const char* root, const char* const* di log_debug("Found %u items in catalog.", ordered_hashmap_size(h)); - items = new(CatalogItem, ordered_hashmap_size(h)); + _cleanup_free_ CatalogItem *items = new(CatalogItem, ordered_hashmap_size(h)); if (!items) return log_oom(); - n = 0; + _cleanup_(strbuf_freep) struct strbuf *sb = strbuf_new(); + if (!sb) + return log_oom(); + + unsigned n = 0; + char *payload; + CatalogItem *i; ORDERED_HASHMAP_FOREACH_KEY(payload, i, h) { log_trace("Found " SD_ID128_FORMAT_STR ", language %s", SD_ID128_FORMAT_VAL(i->id), isempty(i->language) ? "C" : i->language); - offset = strbuf_add_string(sb, payload); + ssize_t offset = strbuf_add_string(sb, payload); if (offset < 0) return log_oom(); @@ -474,7 +500,7 @@ int catalog_update(const char* database, const char* root, const char* const* di strbuf_complete(sb); - sz = write_catalog(database, sb, items, n); + int64_t sz = write_catalog(database, sb, items, n); if (sz < 0) return log_error_errno(sz, "Failed to write %s: %m", database); @@ -483,31 +509,28 @@ int catalog_update(const char* database, const char* root, const char* const* di return 0; } -static int open_mmap(const char *database, int *_fd, struct stat *_st, void **_p) { - _cleanup_close_ int fd = -EBADF; - const CatalogHeader *h; - struct stat st; - void *p; - - assert(_fd); - assert(_st); - assert(_p); +static int open_mmap(const char *database, int *ret_fd, struct stat *ret_st, void **ret_map) { + assert(database); + assert(ret_fd); + assert(ret_st); + assert(ret_map); - fd = open(database, O_RDONLY|O_CLOEXEC); + _cleanup_close_ int fd = open(database, O_RDONLY|O_CLOEXEC); if (fd < 0) return -errno; + struct stat st; if (fstat(fd, &st) < 0) return -errno; if (st.st_size < (off_t) sizeof(CatalogHeader) || file_offset_beyond_memory_size(st.st_size)) return -EINVAL; - p = mmap(NULL, st.st_size, PROT_READ, MAP_SHARED, fd, 0); + void *p = mmap(NULL, st.st_size, PROT_READ, MAP_SHARED, fd, 0); if (p == MAP_FAILED) return -errno; - h = p; + const CatalogHeader *h = p; if (memcmp(h->signature, (const uint8_t[]) CATALOG_SIGNATURE, sizeof(h->signature)) != 0 || le64toh(h->header_size) < sizeof(CatalogHeader) || le64toh(h->catalog_item_size) < sizeof(CatalogItem) || @@ -518,16 +541,15 @@ static int open_mmap(const char *database, int *_fd, struct stat *_st, void **_p return -EBADMSG; } - *_fd = TAKE_FD(fd); - *_st = st; - *_p = p; - + *ret_fd = TAKE_FD(fd); + *ret_st = st; + *ret_map = p; return 0; } -static const char *find_id(void *p, sd_id128_t id) { +static const char* find_id(const void *p, sd_id128_t id) { CatalogItem *f = NULL, key = { .id = id }; - const CatalogHeader *h = p; + const CatalogHeader *h = ASSERT_PTR(p); const char *loc; loc = setlocale(LC_MESSAGES, NULL); @@ -580,33 +602,33 @@ static const char *find_id(void *p, sd_id128_t id) { le64toh(f->offset); } -int catalog_get(const char* database, sd_id128_t id, char **ret_text) { - _cleanup_close_ int fd = -EBADF; - void *p = NULL; - struct stat st; +int catalog_get(const char *database, sd_id128_t id, char **ret_text) { int r; - const char *s; + assert(database); assert(ret_text); + _cleanup_close_ int fd = -EBADF; + struct stat st; + void *p; r = open_mmap(database, &fd, &st, &p); if (r < 0) return r; - s = find_id(p, id); - if (!s) { + const char *s = find_id(p, id); + if (!s) r = -ENOENT; - goto finish; - } + else + r = strdup_to(ret_text, s); - r = strdup_to(ret_text, s); -finish: (void) munmap(p, st.st_size); - return r; } -static char *find_header(const char *s, const char *header) { +static char* find_header(const char *s, const char *header) { + + assert(s); + assert(header); for (;;) { const char *v; @@ -623,6 +645,11 @@ static char *find_header(const char *s, const char *header) { } static void dump_catalog_entry(FILE *f, sd_id128_t id, const char *s, bool oneline) { + assert(s); + + if (!f) + f = stdout; + if (oneline) { _cleanup_free_ char *subject = NULL, *defined_by = NULL; @@ -638,24 +665,23 @@ static void dump_catalog_entry(FILE *f, sd_id128_t id, const char *s, bool oneli } int catalog_list(FILE *f, const char *database, bool oneline) { - _cleanup_close_ int fd = -EBADF; - void *p = NULL; - struct stat st; - const CatalogHeader *h; - const CatalogItem *items; int r; - unsigned n; - sd_id128_t last_id; - bool last_id_set = false; + assert(database); + + _cleanup_close_ int fd = -EBADF; + struct stat st; + void *p; r = open_mmap(database, &fd, &st, &p); if (r < 0) return r; - h = p; - items = (const CatalogItem*) ((const uint8_t*) p + le64toh(h->header_size)); + const CatalogHeader *h = p; + const CatalogItem *items = (const CatalogItem*) ((const uint8_t*) p + le64toh(h->header_size)); - for (n = 0; n < le64toh(h->n_items); n++) { + sd_id128_t last_id; + bool last_id_set = false; + for (size_t n = 0; n < le64toh(h->n_items); n++) { const char *s; if (last_id_set && sd_id128_equal(last_id, items[n].id)) @@ -669,38 +695,33 @@ int catalog_list(FILE *f, const char *database, bool oneline) { last_id = items[n].id; } - munmap(p, st.st_size); - + (void) munmap(p, st.st_size); return 0; } int catalog_list_items(FILE *f, const char *database, bool oneline, char **items) { - int r = 0; + int r, ret = 0; + + assert(database); STRV_FOREACH(item, items) { sd_id128_t id; - int k; - _cleanup_free_ char *msg = NULL; - - k = sd_id128_from_string(*item, &id); - if (k < 0) { - log_error_errno(k, "Failed to parse id128 '%s': %m", *item); - if (r == 0) - r = k; + r = sd_id128_from_string(*item, &id); + if (r < 0) { + RET_GATHER(ret, log_error_errno(r, "Failed to parse id128 '%s': %m", *item)); continue; } - k = catalog_get(database, id, &msg); - if (k < 0) { - log_full_errno(k == -ENOENT ? LOG_NOTICE : LOG_ERR, k, - "Failed to retrieve catalog entry for '%s': %m", *item); - if (r == 0) - r = k; + _cleanup_free_ char *msg = NULL; + r = catalog_get(database, id, &msg); + if (r < 0) { + RET_GATHER(ret, log_full_errno(r == -ENOENT ? LOG_NOTICE : LOG_ERR, r, + "Failed to retrieve catalog entry for '%s': %m", *item)); continue; } dump_catalog_entry(f, id, msg, oneline); } - return r; + return ret; } diff --git a/src/libsystemd/sd-journal/catalog.h b/src/libsystemd/sd-journal/catalog.h index b5a97fa6fbb..e6a8c9b8dbb 100644 --- a/src/libsystemd/sd-journal/catalog.h +++ b/src/libsystemd/sd-journal/catalog.h @@ -7,13 +7,10 @@ #include "sd-id128.h" #include "hashmap.h" -#include "strbuf.h" -int catalog_import_file(OrderedHashmap *h, const char *path); -int catalog_update(const char* database, const char* root, const char* const* dirs); -int catalog_get(const char* database, sd_id128_t id, char **ret_text); -int catalog_list(FILE *f, const char* database, bool oneline); -int catalog_list_items(FILE *f, const char* database, bool oneline, char **items); -int catalog_file_lang(const char *filename, char **lang); -extern const char * const catalog_file_dirs[]; -extern const struct hash_ops catalog_hash_ops; +int catalog_import_file(OrderedHashmap **h, const char *path); +int catalog_update(const char *database, const char *root, const char* const *dirs); +int catalog_get(const char *database, sd_id128_t id, char **ret_text); +int catalog_list(FILE *f, const char *database, bool oneline); +int catalog_list_items(FILE *f, const char *database, bool oneline, char **items); +int catalog_file_lang(const char *filename, char **ret); diff --git a/src/libsystemd/sd-journal/test-catalog.c b/src/libsystemd/sd-journal/test-catalog.c index 603952e904a..ee8bd4e0ec4 100644 --- a/src/libsystemd/sd-journal/test-catalog.c +++ b/src/libsystemd/sd-journal/test-catalog.c @@ -28,31 +28,29 @@ static const char *no_catalog_dirs[] = { static OrderedHashmap* test_import(const char* contents, ssize_t size, int code) { _cleanup_(unlink_tempfilep) char name[] = "/tmp/test-catalog.XXXXXX"; _cleanup_close_ int fd = -EBADF; - OrderedHashmap *h; + OrderedHashmap *h = NULL; if (size < 0) size = strlen(contents); - assert_se(h = ordered_hashmap_new(&catalog_hash_ops)); - fd = mkostemp_safe(name); assert_se(fd >= 0); assert_se(write(fd, contents, size) == size); - assert_se(catalog_import_file(h, name) == code); + assert_se(catalog_import_file(&h, name) == code); return h; } static void test_catalog_import_invalid(void) { - _cleanup_ordered_hashmap_free_free_free_ OrderedHashmap *h = NULL; + _cleanup_ordered_hashmap_free_ OrderedHashmap *h = NULL; h = test_import("xxx", -1, -EINVAL); assert_se(ordered_hashmap_isempty(h)); } static void test_catalog_import_badid(void) { - _unused_ _cleanup_ordered_hashmap_free_free_free_ OrderedHashmap *h = NULL; + _unused_ _cleanup_ordered_hashmap_free_ OrderedHashmap *h = NULL; const char *input = "-- 0027229ca0644181a76c4e92458afaff dededededededededededededededede\n" \ "Subject: message\n" \ @@ -62,7 +60,7 @@ static void test_catalog_import_badid(void) { } static void test_catalog_import_one(void) { - _cleanup_ordered_hashmap_free_free_free_ OrderedHashmap *h = NULL; + _cleanup_ordered_hashmap_free_ OrderedHashmap *h = NULL; char *payload; const char *input = @@ -86,7 +84,7 @@ static void test_catalog_import_one(void) { } static void test_catalog_import_merge(void) { - _cleanup_ordered_hashmap_free_free_free_ OrderedHashmap *h = NULL; + _cleanup_ordered_hashmap_free_ OrderedHashmap *h = NULL; char *payload; const char *input = @@ -118,7 +116,7 @@ static void test_catalog_import_merge(void) { } static void test_catalog_import_merge_no_body(void) { - _cleanup_ordered_hashmap_free_free_free_ OrderedHashmap *h = NULL; + _cleanup_ordered_hashmap_free_ OrderedHashmap *h = NULL; char *payload; const char *input = @@ -222,10 +220,10 @@ int main(int argc, char *argv[]) { test_catalog_update(database); - r = catalog_list(stdout, database, true); + r = catalog_list(NULL, database, true); assert_se(r >= 0); - r = catalog_list(stdout, database, false); + r = catalog_list(NULL, database, false); assert_se(r >= 0); assert_se(catalog_get(database, SD_MESSAGE_COREDUMP, &text) >= 0); -- 2.47.3