From 224b0e7ad0116579f830b7dd387b97c3facb65d9 Mon Sep 17 00:00:00 2001 From: =?utf8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Tue, 28 Nov 2017 12:35:49 +0100 Subject: [PATCH] Add set/hashmap helpers for non-trivial freeing and use where straighforward A macro is needed because otherwise we couldn't ensure type safety. Some simple tests are included. No functional change intended. --- src/basic/hashmap.h | 23 +++++++++++++++++++++++ src/basic/set.h | 12 ++++++++++++ src/busctl/busctl.c | 7 +------ src/cgtop/cgtop.c | 5 +---- src/libsystemd-network/sd-dhcp-server.c | 4 +--- src/libsystemd/sd-bus/bus-track.c | 12 ++---------- src/machine/image-dbus.c | 5 +---- src/machine/machined.c | 6 +----- src/resolve/resolved-dns-scope.c | 7 +------ src/resolve/resolved-dns-trust-anchor.c | 13 ++----------- src/shared/bus-util.c | 7 +------ src/shared/install.c | 23 +++-------------------- src/socket-proxy/socket-proxyd.c | 13 ++----------- src/systemctl/systemctl.c | 7 +------ src/sysusers/sysusers.c | 11 +++-------- src/sysv-generator/sysv-generator.c | 7 +------ src/test/test-hashmap.c | 24 ++++++++++++++++++++++++ src/test/test-set.c | 24 ++++++++++++++++++++++++ src/tmpfiles/tmpfiles.c | 10 ++-------- 19 files changed, 106 insertions(+), 114 deletions(-) diff --git a/src/basic/hashmap.h b/src/basic/hashmap.h index 222da8f..0eb7639 100644 --- a/src/basic/hashmap.h +++ b/src/basic/hashmap.h @@ -329,6 +329,29 @@ static inline void *ordered_hashmap_first(OrderedHashmap *h) { return internal_hashmap_first(HASHMAP_BASE(h)); } +#define hashmap_clear_with_destructor(_s, _f) \ + ({ \ + void *_item; \ + while ((_item = hashmap_steal_first(_s))) \ + _f(_item); \ + }) +#define hashmap_free_with_destructor(_s, _f) \ + ({ \ + hashmap_clear_with_destructor(_s, _f); \ + hashmap_free(_s); \ + }) +#define ordered_hashmap_clear_with_destructor(_s, _f) \ + ({ \ + void *_item; \ + while ((_item = ordered_hashmap_steal_first(_s))) \ + _f(_item); \ + }) +#define ordered_hashmap_free_with_destructor(_s, _f) \ + ({ \ + ordered_hashmap_clear_with_destructor(_s, _f); \ + ordered_hashmap_free(_s); \ + }) + /* no hashmap_next */ void *ordered_hashmap_next(OrderedHashmap *h, const void *key); diff --git a/src/basic/set.h b/src/basic/set.h index e798d5f..156ab4b 100644 --- a/src/basic/set.h +++ b/src/basic/set.h @@ -108,6 +108,18 @@ static inline void *set_steal_first(Set *s) { return internal_hashmap_steal_first(HASHMAP_BASE(s)); } +#define set_clear_with_destructor(_s, _f) \ + ({ \ + void *_item; \ + while ((_item = set_steal_first(_s))) \ + _f(_item); \ + }) +#define set_free_with_destructor(_s, _f) \ + ({ \ + set_clear_with_destructor(_s, _f); \ + set_free(_s); \ + }) + /* no set_steal_first_key */ /* no set_first_key */ diff --git a/src/busctl/busctl.c b/src/busctl/busctl.c index 3b2b5f3..44110b6 100644 --- a/src/busctl/busctl.c +++ b/src/busctl/busctl.c @@ -692,12 +692,7 @@ static void member_free(Member *m) { DEFINE_TRIVIAL_CLEANUP_FUNC(Member*, member_free); static void member_set_free(Set *s) { - Member *m; - - while ((m = set_steal_first(s))) - member_free(m); - - set_free(s); + set_free_with_destructor(s, member_free); } DEFINE_TRIVIAL_CLEANUP_FUNC(Set*, member_set_free); diff --git a/src/cgtop/cgtop.c b/src/cgtop/cgtop.c index 8add7e7..fe339eb 100644 --- a/src/cgtop/cgtop.c +++ b/src/cgtop/cgtop.c @@ -105,10 +105,7 @@ static void group_free(Group *g) { } static void group_hashmap_clear(Hashmap *h) { - Group *g; - - while ((g = hashmap_steal_first(h))) - group_free(g); + hashmap_clear_with_destructor(h, group_free); } static void group_hashmap_free(Hashmap *h) { diff --git a/src/libsystemd-network/sd-dhcp-server.c b/src/libsystemd-network/sd-dhcp-server.c index 660358e..63fb355 100644 --- a/src/libsystemd-network/sd-dhcp-server.c +++ b/src/libsystemd-network/sd-dhcp-server.c @@ -87,7 +87,6 @@ int sd_dhcp_server_configure_pool(sd_dhcp_server *server, struct in_addr *addres size = size_max; if (server->address != address->s_addr || server->netmask != netmask || server->pool_size != size || server->pool_offset != offset) { - DHCPLease *lease; free(server->bound_leases); server->bound_leases = new0(DHCPLease*, size); @@ -105,8 +104,7 @@ int sd_dhcp_server_configure_pool(sd_dhcp_server *server, struct in_addr *addres server->bound_leases[server_off - offset] = &server->invalid_lease; /* Drop any leases associated with the old address range */ - while ((lease = hashmap_steal_first(server->leases_by_client_id))) - dhcp_lease_free(lease); + hashmap_clear_with_destructor(server->leases_by_client_id, dhcp_lease_free); } return 0; diff --git a/src/libsystemd/sd-bus/bus-track.c b/src/libsystemd/sd-bus/bus-track.c index 75930f3..5b55f30 100644 --- a/src/libsystemd/sd-bus/bus-track.c +++ b/src/libsystemd/sd-bus/bus-track.c @@ -184,8 +184,6 @@ _public_ sd_bus_track* sd_bus_track_ref(sd_bus_track *track) { } _public_ sd_bus_track* sd_bus_track_unref(sd_bus_track *track) { - struct track_item *i; - if (!track) return NULL; @@ -196,14 +194,11 @@ _public_ sd_bus_track* sd_bus_track_unref(sd_bus_track *track) { return NULL; } - while ((i = hashmap_steal_first(track->names))) - track_item_free(i); - if (track->in_list) LIST_REMOVE(tracks, track->bus->tracks, track); bus_track_remove_from_queue(track); - hashmap_free(track->names); + hashmap_free_with_destructor(track->names, track_item_free); sd_bus_unref(track->bus); return mfree(track); } @@ -429,8 +424,6 @@ void bus_track_dispatch(sd_bus_track *track) { } void bus_track_close(sd_bus_track *track) { - struct track_item *i; - assert(track); /* Called whenever our bus connected is closed. If so, and our track object is non-empty, dispatch it @@ -448,8 +441,7 @@ void bus_track_close(sd_bus_track *track) { return; /* Let's flush out all names */ - while ((i = hashmap_steal_first(track->names))) - track_item_free(i); + hashmap_clear_with_destructor(track->names, track_item_free); /* Invoke handler */ if (track->handler) diff --git a/src/machine/image-dbus.c b/src/machine/image-dbus.c index aebfc38..10d1b06 100644 --- a/src/machine/image-dbus.c +++ b/src/machine/image-dbus.c @@ -396,14 +396,11 @@ const sd_bus_vtable image_vtable[] = { static int image_flush_cache(sd_event_source *s, void *userdata) { Manager *m = userdata; - Image *i; assert(s); assert(m); - while ((i = hashmap_steal_first(m->image_cache))) - image_unref(i); - + hashmap_clear_with_destructor(m->image_cache, image_unref); return 0; } diff --git a/src/machine/machined.c b/src/machine/machined.c index 17142a6..d481020 100644 --- a/src/machine/machined.c +++ b/src/machine/machined.c @@ -68,7 +68,6 @@ Manager *manager_new(void) { void manager_free(Manager *m) { Machine *machine; - Image *i; assert(m); @@ -84,10 +83,7 @@ void manager_free(Manager *m) { hashmap_free(m->machine_units); hashmap_free(m->machine_leaders); - while ((i = hashmap_steal_first(m->image_cache))) - image_unref(i); - - hashmap_free(m->image_cache); + hashmap_free_with_destructor(m->image_cache, image_unref); sd_event_source_unref(m->image_cache_defer_event); diff --git a/src/resolve/resolved-dns-scope.c b/src/resolve/resolved-dns-scope.c index 4be6040..a9071ee 100644 --- a/src/resolve/resolved-dns-scope.c +++ b/src/resolve/resolved-dns-scope.c @@ -103,8 +103,6 @@ static void dns_scope_abort_transactions(DnsScope *s) { } DnsScope* dns_scope_free(DnsScope *s) { - DnsResourceRecord *rr; - if (!s) return NULL; @@ -119,10 +117,7 @@ DnsScope* dns_scope_free(DnsScope *s) { hashmap_free(s->transactions_by_key); - while ((rr = ordered_hashmap_steal_first(s->conflict_queue))) - dns_resource_record_unref(rr); - - ordered_hashmap_free(s->conflict_queue); + ordered_hashmap_free_with_destructor(s->conflict_queue, dns_resource_record_unref); sd_event_source_unref(s->conflict_event_source); sd_event_source_unref(s->announce_event_source); diff --git a/src/resolve/resolved-dns-trust-anchor.c b/src/resolve/resolved-dns-trust-anchor.c index f2bd9ef..c6e47ed 100644 --- a/src/resolve/resolved-dns-trust-anchor.c +++ b/src/resolve/resolved-dns-trust-anchor.c @@ -542,19 +542,10 @@ int dns_trust_anchor_load(DnsTrustAnchor *d) { } void dns_trust_anchor_flush(DnsTrustAnchor *d) { - DnsAnswer *a; - DnsResourceRecord *rr; - assert(d); - while ((a = hashmap_steal_first(d->positive_by_key))) - dns_answer_unref(a); - d->positive_by_key = hashmap_free(d->positive_by_key); - - while ((rr = set_steal_first(d->revoked_by_rr))) - dns_resource_record_unref(rr); - d->revoked_by_rr = set_free(d->revoked_by_rr); - + d->positive_by_key = hashmap_free_with_destructor(d->positive_by_key, dns_answer_unref); + d->revoked_by_rr = set_free_with_destructor(d->revoked_by_rr, dns_resource_record_unref); d->negative_by_name = set_free_free(d->negative_by_name); } diff --git a/src/shared/bus-util.c b/src/shared/bus-util.c index c6844f5..7d5ea0c 100644 --- a/src/shared/bus-util.c +++ b/src/shared/bus-util.c @@ -554,12 +554,7 @@ int bus_verify_polkit_async( void bus_verify_polkit_async_registry_free(Hashmap *registry) { #if ENABLE_POLKIT - AsyncPolkitQuery *q; - - while ((q = hashmap_steal_first(registry))) - async_polkit_query_free(q); - - hashmap_free(registry); + hashmap_free_with_destructor(registry, async_polkit_query_free); #endif } diff --git a/src/shared/install.c b/src/shared/install.c index a75a045..05ccc58 100644 --- a/src/shared/install.c +++ b/src/shared/install.c @@ -992,23 +992,11 @@ static void install_info_free(UnitFileInstallInfo *i) { free(i); } -static OrderedHashmap* install_info_hashmap_free(OrderedHashmap *m) { - UnitFileInstallInfo *i; - - if (!m) - return NULL; - - while ((i = ordered_hashmap_steal_first(m))) - install_info_free(i); - - return ordered_hashmap_free(m); -} - static void install_context_done(InstallContext *c) { assert(c); - c->will_process = install_info_hashmap_free(c->will_process); - c->have_processed = install_info_hashmap_free(c->have_processed); + c->will_process = ordered_hashmap_free_with_destructor(c->will_process, install_info_free); + c->have_processed = ordered_hashmap_free_with_destructor(c->have_processed, install_info_free); } static UnitFileInstallInfo *install_info_find(InstallContext *c, const char *name) { @@ -3141,12 +3129,7 @@ static void unit_file_list_free_one(UnitFileList *f) { } Hashmap* unit_file_list_free(Hashmap *h) { - UnitFileList *i; - - while ((i = hashmap_steal_first(h))) - unit_file_list_free_one(i); - - return hashmap_free(h); + return hashmap_free_with_destructor(h, unit_file_list_free_one); } DEFINE_TRIVIAL_CLEANUP_FUNC(UnitFileList*, unit_file_list_free_one); diff --git a/src/socket-proxy/socket-proxyd.c b/src/socket-proxy/socket-proxyd.c index 8887e2f..d4f401d 100644 --- a/src/socket-proxy/socket-proxyd.c +++ b/src/socket-proxy/socket-proxyd.c @@ -92,19 +92,10 @@ static void connection_free(Connection *c) { } static void context_free(Context *context) { - sd_event_source *es; - Connection *c; - assert(context); - while ((es = set_steal_first(context->listen))) - sd_event_source_unref(es); - - while ((c = set_first(context->connections))) - connection_free(c); - - set_free(context->listen); - set_free(context->connections); + set_free_with_destructor(context->listen, sd_event_source_unref); + set_free_with_destructor(context->connections, connection_free); sd_event_unref(context->event); sd_resolve_unref(context->resolve); diff --git a/src/systemctl/systemctl.c b/src/systemctl/systemctl.c index 4fd9b10..0d67905 100644 --- a/src/systemctl/systemctl.c +++ b/src/systemctl/systemctl.c @@ -699,12 +699,7 @@ static int get_unit_list( } static void message_set_freep(Set **set) { - sd_bus_message *m; - - while ((m = set_steal_first(*set))) - sd_bus_message_unref(m); - - set_free(*set); + set_free_with_destructor(*set, sd_bus_message_unref); } static int get_unit_list_recursive( diff --git a/src/sysusers/sysusers.c b/src/sysusers/sysusers.c index 909a4ca..d800945 100644 --- a/src/sysusers/sysusers.c +++ b/src/sysusers/sysusers.c @@ -1864,20 +1864,15 @@ int main(int argc, char *argv[]) { log_error_errno(r, "Failed to write files: %m"); finish: - while ((i = hashmap_steal_first(groups))) - item_free(i); - - while ((i = hashmap_steal_first(users))) - item_free(i); + hashmap_free_with_destructor(groups, item_free); + hashmap_free_with_destructor(users, item_free); while ((n = hashmap_first_key(members))) { strv_free(hashmap_steal_first(members)); free(n); } - - hashmap_free(groups); - hashmap_free(users); hashmap_free(members); + hashmap_free(todo_uids); hashmap_free(todo_gids); diff --git a/src/sysv-generator/sysv-generator.c b/src/sysv-generator/sysv-generator.c index cfb34c8..f59c277 100644 --- a/src/sysv-generator/sysv-generator.c +++ b/src/sysv-generator/sysv-generator.c @@ -95,12 +95,7 @@ static void free_sysvstub(SysvStub *s) { DEFINE_TRIVIAL_CLEANUP_FUNC(SysvStub*, free_sysvstub); static void free_sysvstub_hashmapp(Hashmap **h) { - SysvStub *stub; - - while ((stub = hashmap_steal_first(*h))) - free_sysvstub(stub); - - hashmap_free(*h); + hashmap_free_with_destructor(*h, free_sysvstub); } static int add_alias(const char *service, const char *alias) { diff --git a/src/test/test-hashmap.c b/src/test/test-hashmap.c index 6ea1780..dd91954 100644 --- a/src/test/test-hashmap.c +++ b/src/test/test-hashmap.c @@ -38,6 +38,29 @@ static void test_ordered_hashmap_next(void) { assert_se(!ordered_hashmap_next(m, INT_TO_PTR(3))); } +typedef struct Item { + int seen; +} Item; +static void item_seen(Item *item) { + item->seen++; +} + +static void test_hashmap_free_with_destructor(void) { + Hashmap *m; + struct Item items[4] = {}; + unsigned i; + + assert_se(m = hashmap_new(NULL)); + for (i = 0; i < ELEMENTSOF(items) - 1; i++) + assert_se(hashmap_put(m, INT_TO_PTR(i), items + i) == 1); + + m = hashmap_free_with_destructor(m, item_seen); + assert_se(items[0].seen == 1); + assert_se(items[1].seen == 1); + assert_se(items[2].seen == 1); + assert_se(items[3].seen == 0); +} + static void test_uint64_compare_func(void) { const uint64_t a = 0x100, b = 0x101; @@ -62,6 +85,7 @@ int main(int argc, const char *argv[]) { test_ordered_hashmap_funcs(); test_ordered_hashmap_next(); + test_hashmap_free_with_destructor(); test_uint64_compare_func(); test_trivial_compare_func(); test_string_compare_func(); diff --git a/src/test/test-set.c b/src/test/test-set.c index 7bae95b..0a29a62 100644 --- a/src/test/test-set.c +++ b/src/test/test-set.c @@ -40,6 +40,29 @@ static void test_set_steal_first(void) { assert_se(set_isempty(m)); } +typedef struct Item { + int seen; +} Item; +static void item_seen(Item *item) { + item->seen++; +} + +static void test_set_free_with_destructor(void) { + Set *m; + struct Item items[4] = {}; + unsigned i; + + assert_se(m = set_new(NULL)); + for (i = 0; i < ELEMENTSOF(items) - 1; i++) + assert_se(set_put(m, items + i) == 1); + + m = set_free_with_destructor(m, item_seen); + assert_se(items[0].seen == 1); + assert_se(items[1].seen == 1); + assert_se(items[2].seen == 1); + assert_se(items[3].seen == 0); +} + static void test_set_put(void) { _cleanup_set_free_ Set *m = NULL; @@ -102,6 +125,7 @@ static void test_set_make(void) { int main(int argc, const char *argv[]) { test_set_steal_first(); + test_set_free_with_destructor(); test_set_put(); test_set_make(); diff --git a/src/tmpfiles/tmpfiles.c b/src/tmpfiles/tmpfiles.c index 970ea3b..ad89b46 100644 --- a/src/tmpfiles/tmpfiles.c +++ b/src/tmpfiles/tmpfiles.c @@ -2393,14 +2393,8 @@ int main(int argc, char *argv[]) { } finish: - while ((a = ordered_hashmap_steal_first(items))) - item_array_free(a); - - while ((a = ordered_hashmap_steal_first(globs))) - item_array_free(a); - - ordered_hashmap_free(items); - ordered_hashmap_free(globs); + ordered_hashmap_free_with_destructor(items, item_array_free); + ordered_hashmap_free_with_destructor(globs, item_array_free); free(arg_include_prefixes); free(arg_exclude_prefixes); -- 2.7.4