Bug 39609

Review of attachment 49635 [details] [review]:

Am I right in thinking this is a circular build-dependency, so to bootstrap a new architecture in a systemd-aware way, you have to build libdbus without systemd support, build systemd against that, then rebuild dbus with systemd support?

(Not necessarily a problem, but it needs documenting somewhere. We could mention the dbus/dbus-glib circular build dependency when all tests are explicitly enabled, in the same place.)

::: configure.ac
@@ +1078,3 @@
+        have_systemd=no)
+    AC_SUBST(SYSTEMD_CFLAGS)
+    AC_SUBST(SYSTEMD_LIBS)

If you explicitly say "--enable-systemd=yes", then configure should fail hard if it's missing.

Review of attachment 49635 [details] [review]:

::: dbus/dbus-userdb-util.c
@@ +65,3 @@
+      /* Check whether this user is logged in on at least one physical
+         seat */
+      r = sd_uid_get_seats (uid, 0, &seats);

Would it be worth adding a sd_uid_has_a_seat(), that doesn't copy an array of seats just to free them again?

@@ +70,3 @@
+          dbus_set_error (error, _dbus_error_from_errno (-r),
+                          "Failed to determine seats of user \"%lu\": %s",
+                          (unsigned long) uid,

Please use DBUS_UID_FORMAT in the format string, then you won't have to cast

@@ +88,3 @@
+
+          free (seats);
+          have_seats = k > 0;

I'd prefer "= (k > 0)"

@@ +485,3 @@
+  dbus_error_init (&error);
+  printf ("Is Console user: %i\n",
+          _dbus_is_console_user(uid, &error));

Space before parenthesis, please

(In reply to comment #1)
> Am I right in thinking this is a circular build-dependency, so to bootstrap a
> new architecture in a systemd-aware way, you have to build libdbus without
> systemd support, build systemd against that, then rebuild dbus with systemd
> support?
> 
> (Not necessarily a problem, but it needs documenting somewhere. We could
> mention the dbus/dbus-glib circular build dependency when all tests are
> explicitly enabled, in the same place.)

I've added some information about this to README; please add any systemd bootstrapping notes there.

Created attachment 50548 [details] [review]
updated patch

Updated version fixing all issues pointed out.

(In reply to comment #1)
> Review of attachment 49635 [details] [review]:
> 
> Am I right in thinking this is a circular build-dependency, so to bootstrap a
> new architecture in a systemd-aware way, you have to build libdbus without
> systemd support, build systemd against that, then rebuild dbus with systemd
> support?
> 
> (Not necessarily a problem, but it needs documenting somewhere. We could
> mention the dbus/dbus-glib circular build dependency when all tests are
> explicitly enabled, in the same place.)

I have documented the cyclic build dep in systemd's README now, since that appear the more more appropriate place to me, after all systemd pulls in dbus, not the other way round. I don't think an additional note about this in D-Bus is necessary.

> 
> ::: configure.ac
> @@ +1078,3 @@
> +        have_systemd=no)
> +    AC_SUBST(SYSTEMD_CFLAGS)
> +    AC_SUBST(SYSTEMD_LIBS)
> 
> If you explicitly say "--enable-systemd=yes", then configure should fail hard
> if it's missing.

Fixed. (I had just copied the style of the existing PKG_CONFIG blocks which don't do checks like this...)

Created attachment 50549 [details] [review]
updated patch, second try

Apparently I uploaded the wrong patch... Here's another try.

Created attachment 50550 [details] [review]
Third try

I am an idiot. But this one should be the right patch.

(In reply to comment #2)
> Review of attachment 49635 [details] [review]:
> 
> ::: dbus/dbus-userdb-util.c
> @@ +65,3 @@
> +      /* Check whether this user is logged in on at least one physical
> +         seat */
> +      r = sd_uid_get_seats (uid, 0, &seats);
> 
> Would it be worth adding a sd_uid_has_a_seat(), that doesn't copy an array of
> seats just to free them again?

I have now modified the library to return only the count of sounds if the seats param is NULL. This makes it unnecessary to free the string array.

> @@ +70,3 @@
> +          dbus_set_error (error, _dbus_error_from_errno (-r),
> +                          "Failed to determine seats of user \"%lu\": %s",
> +                          (unsigned long) uid,
> 
> Please use DBUS_UID_FORMAT in the format string, then you won't have to cast

Fixed.

> @@ +88,3 @@
> +
> +          free (seats);
> +          have_seats = k > 0;
> 
> I'd prefer "= (k > 0)"

Meh, but changed.
 
> @@ +485,3 @@
> +  dbus_error_init (&error);
> +  printf ("Is Console user: %i\n",
> +          _dbus_is_console_user(uid, &error));
> 
> Space before parenthesis, please

Changed.

Created attachment 50551 [details] [review]
patch dropping stdlib.h inclusion

Minor update: we don't need to include stdlib.h anymore since we don't need glibc free().

Review of attachment 50551 [details] [review]:

Ok, this looks shockingly simple.  Clearly a lot of logic lives in systemd.  ...looks for it...  Yeah, looks generally OK.

It's a bit surprising to me that the src/ is all one directory...you could use a README file or something per component.  But anyways, patch here looks good.

Review of attachment 50551 [details] [review]:

::: configure.ac
@@ +1148,3 @@
 #### Set up final flags
+DBUS_CLIENT_CFLAGS="$SYSTEMD_CFLAGS"
+DBUS_CLIENT_LIBS="$THREAD_LIBS $NETWORK_libs $SYSTEMD_LIBS"

Please don't add SYSTEMD_CFLAGS to DBUS_CLIENT_CFLAGS, and similar for LIBS: these are used for libdbus, but the systemd integration is in a -util.c file, which are not in the shared libdbus, only in the static libdbus-internal used by the bus daemon and tools.

Add SYSTEMD_CFLAGS and SYSTEMD_LIBS to the right variables in Makefile.am instead (for the latter, only put them in libdbus_internal_la_LIBADD, not in libdbus_la_LIBADD).

(I don't think we want systemd ending up (conditionally) in the pkg-config file for the shared libdbus, given that that library never uses systemd.)

(In reply to comment #11)
> Review of attachment 50551 [details] [review]:
> 
> ::: configure.ac
> @@ +1148,3 @@
>  #### Set up final flags
> +DBUS_CLIENT_CFLAGS="$SYSTEMD_CFLAGS"
> +DBUS_CLIENT_LIBS="$THREAD_LIBS $NETWORK_libs $SYSTEMD_LIBS"
> 
> Please don't add SYSTEMD_CFLAGS to DBUS_CLIENT_CFLAGS, and similar for LIBS:
> these are used for libdbus, but the systemd integration is in a -util.c file,
> which are not in the shared libdbus, only in the static libdbus-internal used
> by the bus daemon and tools.
> 
> Add SYSTEMD_CFLAGS and SYSTEMD_LIBS to the right variables in Makefile.am
> instead (for the latter, only put them in libdbus_internal_la_LIBADD, not in
> libdbus_la_LIBADD).
> 
> (I don't think we want systemd ending up (conditionally) in the pkg-config file
> for the shared libdbus, given that that library never uses systemd.)

Fixed.

Created attachment 50606 [details] [review]
newest patch version, this time adding $(SYSTEMD_LIBS) in Makefile.am

Created attachment 56491 [details] [review]
Rebased to current git

Comment on attachment 56491 [details] [review]
Rebased to current git

Review of attachment 56491 [details] [review]:
-----------------------------------------------------------------

Fixed in master, will be in 1.5.10.

::: configure.ac
@@ +1109,5 @@
> +if test x$enable_systemd = xno ; then
> +    have_systemd=no;
> +else
> +    PKG_CHECK_MODULES(SYSTEMD,
> +        [ libsystemd-login libsystemd-daemon ],

This should really have checked for a version of systemd new enough that sd_uid_get_seats (., ., NULL) is considered valid, which according to your gitweb is >= 32. Rather than have another review round-trip, I just fixed it myself.

(In reply to comment #15)
> Comment on attachment 56491 [details] [review] [review]
> Rebased to current git
> 
> Review of attachment 56491 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> Fixed in master, will be in 1.5.10.
> 
> ::: configure.ac
> @@ +1109,5 @@
> > +if test x$enable_systemd = xno ; then
> > +    have_systemd=no;
> > +else
> > +    PKG_CHECK_MODULES(SYSTEMD,
> > +        [ libsystemd-login libsystemd-daemon ],
> 
> This should really have checked for a version of systemd new enough that
> sd_uid_get_seats (., ., NULL) is considered valid, which according to your
> gitweb is >= 32. Rather than have another review round-trip, I just fixed it
> myself.

Awesome, thanks!