From 6bb3ea655d08c0602c99ccd2a580ba102fd19114 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Thu, 30 May 2024 11:33:20 +0200 Subject: [PATCH 1/4] TEST-58-REPART: reverse order of diff args I expect the test output to be the second argument, so we're diffing "expected" and "output", not the other way around. I noticed this when working on https://github.com/systemd/systemd/pull/33081. --- test/units/TEST-58-REPART.sh | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/test/units/TEST-58-REPART.sh b/test/units/TEST-58-REPART.sh index fc06fde0ee..77adce47bb 100755 --- a/test/units/TEST-58-REPART.sh +++ b/test/units/TEST-58-REPART.sh @@ -423,7 +423,7 @@ EOF --json=pretty \ "$imgs/zzz") - diff -u <(echo "$output") - < Date: Thu, 6 Jun 2024 13:23:17 +0200 Subject: [PATCH 2/4] mkosi: move variable to the right scope --- mkosi.images/system/mkosi.sync | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/mkosi.images/system/mkosi.sync b/mkosi.images/system/mkosi.sync index d21ecd1980..fef0e3b138 100755 --- a/mkosi.images/system/mkosi.sync +++ b/mkosi.images/system/mkosi.sync @@ -13,16 +13,16 @@ if [[ -d "$PKG_SUBDIR/.git" ]] && [[ "$(git -C "$PKG_SUBDIR" rev-parse HEAD)" == exit 0 fi -# The repository on Salsa has the full upstream sources, so it's a waste of space to -# redownload and duplicate everything, so do a sparse checkout as we only need the -# packaging directory anyway -if [[ -n "${GIT_SUBDIR:-}" ]]; then - sparse=(--no-checkout --filter=tree:0) -else - sparse=() -fi - if [[ ! -e "$PKG_SUBDIR" ]] || [[ -z "$(ls --almost-all "$PKG_SUBDIR")" ]]; then + # The repository on Salsa has the full upstream sources, so it's a waste of + # space to redownload and duplicate everything, so do a sparse checkout as + # we only need the packaging directory anyway. + if [[ -n "${GIT_SUBDIR:-}" ]]; then + sparse=(--no-checkout --filter=tree:0) + else + sparse=() + fi + git clone "$GIT_URL" --branch "$GIT_BRANCH" "${sparse[@]}" "$PKG_SUBDIR" if [[ -n "${GIT_SUBDIR:-}" ]]; then # --no-cone is needed to check out only one top-level directory From 6af868a229169e8735115457132da84c1e54bae6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Wed, 26 Jun 2024 13:07:03 +0200 Subject: [PATCH 3/4] sd-bus: simplify handling of 'types' in convenience methods The arg types==NULL has different meanings for different functions. Some functions like sd_bus_message_appendv() require a non-null param and treat "" as "no data". Other functions like sd_bus_skip() treat null as "process one item", while the convenience functions treat NULL the same as "". So I think it's reasonable to make the convenience functions handle NULL explicitly, separately from "". That way the logical separation of concerns is clearer, and e.g. sd_bus_message_appendv() handles all non-null strings, while e.g. sd_bus_call_methodv() doesn't look into the string at all. Behaviour is unchanged. --- src/libsystemd/sd-bus/bus-convenience.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/libsystemd/sd-bus/bus-convenience.c b/src/libsystemd/sd-bus/bus-convenience.c index 14d8073bc9..e9a350bad6 100644 --- a/src/libsystemd/sd-bus/bus-convenience.c +++ b/src/libsystemd/sd-bus/bus-convenience.c @@ -39,7 +39,7 @@ _public_ int sd_bus_emit_signal_tov( if (r < 0) return r; - if (!isempty(types)) { + if (types) { r = sd_bus_message_appendv(m, types, ap); if (r < 0) return r; @@ -118,7 +118,7 @@ _public_ int sd_bus_call_method_asyncv( if (r < 0) return r; - if (!isempty(types)) { + if (types) { r = sd_bus_message_appendv(m, types, ap); if (r < 0) return r; @@ -174,7 +174,7 @@ _public_ int sd_bus_call_methodv( if (r < 0) goto fail; - if (!isempty(types)) { + if (types) { r = sd_bus_message_appendv(m, types, ap); if (r < 0) goto fail; @@ -229,7 +229,7 @@ _public_ int sd_bus_reply_method_returnv( if (r < 0) return r; - if (!isempty(types)) { + if (types) { r = sd_bus_message_appendv(m, types, ap); if (r < 0) return r; From 519ae503e351dff5a6108a13d37ceaab5eb9cdf7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Fri, 28 Jun 2024 11:45:41 +0200 Subject: [PATCH 4/4] sd-bus: drop bytefield annontations It's the same old story: 'struct sd_bus' is generally instantiated once, so bitfields, for which we pay with more complicated code in all users of this struct, are counterproductive. In some progs the structure may be instantiated a few times, but it's still not worth it because we save a few bytes of memory in one place and pay for this with many more bytes in the code. $ size build/libsystemd.so.0.39.0{.orig,} text data bss dec hex filename 2452757 65376 3768 2521901 267b2d build/libsystemd.so.0.39.0.orig 2451669 65376 3768 2520813 2676ed build/libsystemd.so.0.39.0 $ diff -u <(pahole build/libsystemd.so.0.39.0.orig) <(pahole build/libsystemd.so.0.39.0) ... - /* size: 1960, cachelines: 31, members: 105 */ - /* sum members: 1944, holes: 3, sum holes: 9 */ - /* sum bitfield members: 25 bits, bit holes: 2, sum bit holes: 31 bits */ + /* size: 1984, cachelines: 31, members: 105 */ + /* sum members: 1971, holes: 4, sum holes: 13 */ /* member types with holes: 1, total: 1 */ i.e. 2452757 - 2451669 = 1088 extra bytes of code and slower execution, to save 24 bytes of memory per instance of the struct. (But the number of cachelines doesn't change, so the smaller struct most likely has no effect on memory access, and the alignment of the struct most likely means that the memory saving is illusory too, we just end up with a few bytes of padding after the struct.) In the other structs, the alignment prevent the bitfield for having any effect on memory use, but the compiler would still generate more complicated code, i.e. we pay something for nothing. For example: $ diff -u <(pahole build/libsystemd.so.0.39.0.orig) <(pahole build/libsystemd.so.0.39.0) ... struct node_callback { struct node * node; /* 0 8 */ - _Bool is_fallback:1; /* 8: 0 1 */ + _Bool is_fallback; /* 8 1 */ - /* XXX 7 bits hole, try to pack */ /* XXX 3 bytes hole, try to pack */ unsigned int last_iteration; /* 12 4 */ @@ -455,15 +448,13 @@ struct node_callback * callbacks_prev; /* 32 8 */ /* size: 40, cachelines: 1, members: 6 */ - /* sum members: 36, holes: 1, sum holes: 3 */ - /* sum bitfield members: 1 bits, bit holes: 1, sum bit holes: 7 bits */ + /* sum members: 37, holes: 1, sum holes: 3 */ /* last cacheline: 40 bytes */ }; I kept the bitfield in sd_bus_slot because it prevents the struct from growing from 112 to 120 bytes by reducing the alignment requirement for subsequent fields, and we potentially can have this instantiated many times. --- src/libsystemd/sd-bus/bus-internal.h | 52 ++++++++++++++-------------- src/libsystemd/sd-bus/sd-bus.c | 2 +- 2 files changed, 27 insertions(+), 27 deletions(-) diff --git a/src/libsystemd/sd-bus/bus-internal.h b/src/libsystemd/sd-bus/bus-internal.h index 0d99d2d654..5b3cae759b 100644 --- a/src/libsystemd/sd-bus/bus-internal.h +++ b/src/libsystemd/sd-bus/bus-internal.h @@ -69,7 +69,7 @@ struct node { struct node_callback { struct node *node; - bool is_fallback:1; + bool is_fallback; unsigned last_iteration; sd_bus_message_handler_t callback; @@ -96,7 +96,7 @@ struct node_object_manager { struct node_vtable { struct node *node; - bool is_fallback:1; + bool is_fallback; unsigned last_iteration; char *interface; @@ -190,33 +190,33 @@ struct sd_bus { int message_version; int message_endian; - bool can_fds:1; - bool bus_client:1; - bool ucred_valid:1; - bool is_server:1; - bool anonymous_auth:1; - bool prefer_readv:1; - bool prefer_writev:1; - bool match_callbacks_modified:1; - bool filter_callbacks_modified:1; - bool nodes_modified:1; - bool trusted:1; - bool manual_peer_interface:1; - bool allow_interactive_authorization:1; - bool exit_on_disconnect:1; - bool exited:1; - bool exit_triggered:1; - bool is_local:1; - bool watch_bind:1; - bool is_monitor:1; - bool accept_fd:1; - bool attach_timestamp:1; - bool connected_signal:1; - bool close_on_exit:1; + bool can_fds; + bool bus_client; + bool ucred_valid; + bool is_server; + bool anonymous_auth; + bool prefer_readv; + bool prefer_writev; + bool match_callbacks_modified; + bool filter_callbacks_modified; + bool nodes_modified; + bool trusted; + bool manual_peer_interface; + bool allow_interactive_authorization; + bool exit_on_disconnect; + bool exited; + bool exit_triggered; + bool is_local; + bool watch_bind; + bool is_monitor; + bool accept_fd; + bool attach_timestamp; + bool connected_signal; + bool close_on_exit; RuntimeScope runtime_scope; - signed int use_memfd:2; + int use_memfd; void *rbuffer; size_t rbuffer_size; diff --git a/src/libsystemd/sd-bus/sd-bus.c b/src/libsystemd/sd-bus/sd-bus.c index 738afe0e94..2683959db5 100644 --- a/src/libsystemd/sd-bus/sd-bus.c +++ b/src/libsystemd/sd-bus/sd-bus.c @@ -3673,7 +3673,7 @@ static int io_callback(sd_event_source *s, int fd, uint32_t revents, void *userd sd_bus *bus = ASSERT_PTR(userdata); int r; - /* Note that this is called both on input_fd, output_fd as well as inotify_fd events */ + /* Note that this is called both on input_fd, output_fd, as well as inotify_fd events */ r = sd_bus_process(bus, NULL); if (r < 0) {