Merge pull request #12526 from keszybz/some-trivial-follow-ups-for-the-varlink-pr

Some trivial follow ups for the varlink PR

coccinelle/sd_event_source_disable_unref.cocci

@@ -0,0 +1,36 @@
+@@
+expression p;
+@@
+- if (p) {
+-         (void) sd_event_source_set_enabled(p, SD_EVENT_OFF);
+-         p = sd_event_source_unref(p);
+- }
++ p = sd_event_source_disable_unref(p);
+@@
+expression p;
+@@
+- if (p) {
+-         sd_event_source_set_enabled(p, SD_EVENT_OFF);
+-         sd_event_source_unref(p);
+- }
++ sd_event_source_disable_unref(p);
+@@
+expression p;
+@@
+- if (p) {
+-         (void) sd_event_source_set_enabled(p, SD_EVENT_OFF);
+-         sd_event_source_unref(p);
+- }
++ sd_event_source_disable_unref(p);
+@@
+expression p;
+@@
+- (void) sd_event_source_set_enabled(p, SD_EVENT_OFF);
+- sd_event_source_unref(p);
++ sd_event_source_disable_unref(p);
+@@
+expression p;
+@@
+- sd_event_source_set_enabled(p, SD_EVENT_OFF);
+- sd_event_source_unref(p);
++ sd_event_source_disable_unref(p);

man/rules/meson.build

@@ -445,7 +445,10 @@ manpages = [
  ['sd_event_source_set_userdata', '3', ['sd_event_source_get_userdata'], ''],
  ['sd_event_source_unref',
   '3',
-  ['sd_event_source_ref', 'sd_event_source_unrefp'],
+  ['sd_event_source_disable_unref',
+   'sd_event_source_disable_unrefp',
+   'sd_event_source_ref',
+   'sd_event_source_unrefp'],
   ''],
  ['sd_event_wait',
   '3',

man/sd_event_source_unref.xml

@@ -19,6 +19,8 @@
     <refname>sd_event_source_unref</refname>
     <refname>sd_event_source_unrefp</refname>
     <refname>sd_event_source_ref</refname>
+    <refname>sd_event_source_disable_unref</refname>
+    <refname>sd_event_source_disable_unrefp</refname>
 
     <refpurpose>Increase or decrease event source reference counters</refpurpose>
   </refnamediv>
@@ -42,6 +44,15 @@
         <paramdef>sd_event_source *<parameter>source</parameter></paramdef>
       </funcprototype>
 
+      <funcprototype>
+        <funcdef>sd_event_source* <function>sd_event_source_disable_unref</function></funcdef>
+        <paramdef>sd_event_source *<parameter>source</parameter></paramdef>
+      </funcprototype>
+
+      <funcprototype>
+        <funcdef>void <function>sd_event_source_disable_unrefp</function></funcdef>
+        <paramdef>sd_event_source **<parameter>source</parameter></paramdef>
+      </funcprototype>
     </funcsynopsis>
   </refsynopsisdiv>
 
@@ -77,23 +88,32 @@
     the passed event source object is
     <constant>NULL</constant>.</para>
 
-    <para>Note that event source objects stay alive and may be
+    <para>Note that event source objects stay alive and may be dispatched as long as they have a reference
-    dispatched as long as they have a reference counter greater than
+    counter greater than zero. In order to drop a reference of an event source and make sure the associated
-    zero. In order to drop a reference of an event source and make
+    event source handler function is not called anymore it is recommended to combine a call of
-    sure the associated event source handler function is not called
-    anymore it is recommended to combine a call of
     <function>sd_event_source_unref()</function> with a prior call to
-    <function>sd_event_source_set_enabled()</function> with
+    <function>sd_event_source_set_enabled()</function> with <constant>SD_EVENT_OFF</constant> or call
-    <constant>SD_EVENT_OFF</constant>.</para>
+    <function>sd_event_source_disable_unref()</function>, see below.</para>
+
+    <para><function>sd_event_source_disable_unref()</function> combines a call to
+    <function>sd_event_source_set_enabled()</function> with <constant>SD_EVENT_OFF</constant> with
+    <function>sd_event_source_unref()</function>. This ensures that the source is disabled before the local
+    reference to it is lost. The <parameter>source</parameter> parameter is allowed to be
+    <constant>NULL</constant>.</para>
+
+    <para><function>sd_event_source_disable_unrefp()</function> is similar to
+    <function>sd_event_source_unrefp()</function>, but in addition disables the source first. This call is
+    useful in conjunction with GCC's and LLVM's
+    <ulink url="https://gcc.gnu.org/onlinedocs/gcc/Common-Variable-Attributes.html">Clean-up Variable
+    Attribute</ulink>. Note that this function is defined as inline function.</para>
   </refsect1>
 
   <refsect1>
     <title>Return Value</title>
 
-    <para><function>sd_event_source_unref()</function> always returns
+    <para><function>sd_event_source_unref()</function> and
-    <constant>NULL</constant>.
+    <function>sd_event_source_disable_unref()</function> always return <constant>NULL</constant>.
-    <function>sd_event_source_ref()</function> always returns the
+    <function>sd_event_source_ref()</function> always returns the event source object passed in.</para>
-    event source object passed in.</para>
   </refsect1>
 
   <xi:include href="libsystemd-pkgconfig.xml" />

src/core/service.c

@@ -320,10 +320,7 @@ static void service_fd_store_unlink(ServiceFDStore *fs) {
                 fs->service->n_fd_store--;
         }
 
-        if (fs->event_source) {
+        sd_event_source_disable_unref(fs->event_source);
-                sd_event_source_set_enabled(fs->event_source, SD_EVENT_OFF);
-                sd_event_source_unref(fs->event_source);
-        }
 
         free(fs->fdname);
         safe_close(fs->fd);

src/import/curl-util.c

@@ -70,8 +70,7 @@ static int curl_glue_socket_callback(CURLM *curl, curl_socket_t s, int action, v
                         fd = sd_event_source_get_io_fd(io);
                         assert(fd >= 0);
 
-                        sd_event_source_set_enabled(io, SD_EVENT_OFF);
+                        sd_event_source_disable_unref(io);
-                        sd_event_source_unref(io);
 
                         hashmap_remove(g->ios, FD_TO_PTR(s));
                         hashmap_remove(g->translate_fds, FD_TO_PTR(fd));

src/journal/journal-file.c

@@ -357,8 +357,7 @@ JournalFile* journal_file_close(JournalFile *f) {
                 if (sd_event_source_get_enabled(f->post_change_timer, NULL) > 0)
                         journal_file_post_change(f);
 
-                (void) sd_event_source_set_enabled(f->post_change_timer, SD_EVENT_OFF);
+                sd_event_source_disable_unref(f->post_change_timer);
-                sd_event_source_unref(f->post_change_timer);
         }
 
         journal_file_set_offline(f, true);

src/journal/journalctl.c

@@ -1949,11 +1949,13 @@ static int simple_varlink_call(const char *option, const char *method) {
 
         r = varlink_connect_address(&link, "/run/systemd/journal/io.systemd.journal");
         if (r < 0)
-                return log_error_errno(r, "Failed to connect to journal: %m");
+                return log_error_errno(r, "Failed to connect to /run/systemd/journal/io.systemd.journal: %m");
+
+        (void) varlink_set_description(link, "journal");
 
         r = varlink_call(link, method, NULL, NULL, &error, NULL);
         if (r < 0)
-                return log_error_errno(r, "Failed to execute operation: %s", error);
+                return log_error_errno(r, "Failed to execute varlink call: %s", error);
 
         return 0;
 }

src/libsystemd/libsystemd.sym

@@ -680,4 +680,5 @@ global:
 LIBSYSTEMD_243 {
 global:
         sd_bus_object_vtable_format;
+        sd_event_source_disable_unref;
 } LIBSYSTEMD_241;

src/libsystemd/sd-event/sd-event.c

@@ -339,6 +339,12 @@ fail:
 
 DEFINE_PUBLIC_TRIVIAL_REF_UNREF_FUNC(sd_event, sd_event, event_free);
 
+_public_ sd_event_source* sd_event_source_disable_unref(sd_event_source *s) {
+        if (s)
+                (void) sd_event_source_set_enabled(s, SD_EVENT_OFF);
+        return sd_event_source_unref(s);
+}
+
 static bool event_pid_changed(sd_event *e) {
         assert(e);
 

src/shared/varlink.c

@@ -228,10 +228,15 @@ static inline const char *varlink_server_description(VarlinkServer *s) {
 
 static void varlink_set_state(Varlink *v, VarlinkState state) {
         assert(v);
+        assert(state >= 0 && state < _VARLINK_STATE_MAX);
 
-        varlink_log(v, "varlink: changing state %s → %s",
+        if (v->state < 0)
-                  varlink_state_to_string(v->state),
+                varlink_log(v, "varlink: setting state %s",
-                  varlink_state_to_string(state));
+                            varlink_state_to_string(state));
+        else
+                varlink_log(v, "varlink: changing state %s → %s",
+                            varlink_state_to_string(v->state),
+                            varlink_state_to_string(state));
 
         v->state = state;
 }
@@ -335,25 +340,13 @@ int varlink_connect_fd(Varlink **ret, int fd) {
 static void varlink_detach_event_sources(Varlink *v) {
         assert(v);
 
-        if (v->io_event_source) {
+        v->io_event_source = sd_event_source_disable_unref(v->io_event_source);
-                (void) sd_event_source_set_enabled(v->io_event_source, SD_EVENT_OFF);
-                v->io_event_source = sd_event_source_unref(v->io_event_source);
-        }
 
-        if (v->time_event_source) {
+        v->time_event_source = sd_event_source_disable_unref(v->time_event_source);
-                (void) sd_event_source_set_enabled(v->time_event_source, SD_EVENT_OFF);
-                v->time_event_source = sd_event_source_unref(v->time_event_source);
-        }
 
-        if (v->quit_event_source) {
+        v->quit_event_source = sd_event_source_disable_unref(v->quit_event_source);
-                (void) sd_event_source_set_enabled(v->quit_event_source, SD_EVENT_OFF);
-                v->quit_event_source = sd_event_source_unref(v->quit_event_source);
-        }
 
-        if (v->defer_event_source) {
+        v->defer_event_source = sd_event_source_disable_unref(v->defer_event_source);
-                (void) sd_event_source_set_enabled(v->defer_event_source, SD_EVENT_OFF);
-                v->defer_event_source = sd_event_source_unref(v->defer_event_source);
-        }
 }
 
 static void varlink_clear(Varlink *v) {
@@ -2203,10 +2196,7 @@ static VarlinkServerSocket* varlink_server_socket_destroy(VarlinkServerSocket *s
         if (ss->server)
                 LIST_REMOVE(sockets, ss->server->sockets, ss);
 
-        if (ss->event_source) {
+        sd_event_source_disable_unref(ss->event_source);
-                (void) sd_event_source_set_enabled(ss->event_source, SD_EVENT_OFF);
-                sd_event_source_unref(ss->event_source);
-        }
 
         free(ss->address);
         safe_close(ss->fd);

src/systemd/sd-event.h

@@ -113,6 +113,7 @@ int sd_event_get_iteration(sd_event *e, uint64_t *ret);
 
 sd_event_source* sd_event_source_ref(sd_event_source *s);
 sd_event_source* sd_event_source_unref(sd_event_source *s);
+sd_event_source* sd_event_source_disable_unref(sd_event_source *s);
 
 sd_event *sd_event_source_get_event(sd_event_source *s);
 void* sd_event_source_get_userdata(sd_event_source *s);
@@ -149,6 +150,7 @@ int sd_event_source_set_floating(sd_event_source *s, int b);
 /* Define helpers so that __attribute__((cleanup(sd_event_unrefp))) and similar may be used. */
 _SD_DEFINE_POINTER_CLEANUP_FUNC(sd_event, sd_event_unref);
 _SD_DEFINE_POINTER_CLEANUP_FUNC(sd_event_source, sd_event_source_unref);
+_SD_DEFINE_POINTER_CLEANUP_FUNC(sd_event_source, sd_event_source_disable_unref);
 
 _SD_END_DECLARATIONS;
 

src/test/test-alloc-util.c

@@ -6,6 +6,7 @@
 #include "alloc-util.h"
 #include "macro.h"
 #include "memory-util.h"
+#include "tests.h"
 
 static void test_alloca(void) {
         static const uint8_t zero[997] = { };
@@ -106,11 +107,39 @@ static void test_bool_assign(void) {
         assert(!h);
 }
 
+static int cleanup_counter = 0;
+
+static void cleanup1(void *a) {
+        log_info("%s(%p)", __func__, a);
+        assert_se(++cleanup_counter == *(int*) a);
+}
+static void cleanup2(void *a) {
+        log_info("%s(%p)", __func__, a);
+        assert_se(++cleanup_counter == *(int*) a);
+}
+static void cleanup3(void *a) {
+        log_info("%s(%p)", __func__, a);
+        assert_se(++cleanup_counter == *(int*) a);
+}
+
+static void test_cleanup_order(void) {
+        _cleanup_(cleanup1) int x1 = 4, x2 = 3;
+        _cleanup_(cleanup3) int z = 2;
+        _cleanup_(cleanup2) int y = 1;
+        log_debug("x1: %p", &x1);
+        log_debug("x2: %p", &x2);
+        log_debug("y: %p", &y);
+        log_debug("z: %p", &z);
+}
+
 int main(int argc, char *argv[]) {
+        test_setup_logging(LOG_DEBUG);
+
         test_alloca();
         test_GREEDY_REALLOC();
         test_memdup_multiply_and_greedy_realloc();
         test_bool_assign();
+        test_cleanup_order();
 
         return 0;
 }