Bug 39609 - add systemd backend for at_console policy checks
Summary: add systemd backend for at_console policy checks
Status: RESOLVED FIXED
Alias: None
Product: dbus
Classification: Unclassified
Component: core (show other bugs)
Version: unspecified
Hardware: Other All
: medium normal
Assignee: Havoc Pennington
QA Contact: John (J5) Palmieri
URL:
Whiteboard: review-, but looks OK in principle
Keywords: patch
Depends on:
Blocks:
 
Reported: 2011-07-27 19:37 UTC by Lennart Poettering
Modified: 2012-02-09 10:42 UTC (History)
1 user (show)

See Also:
i915 platform:
i915 features:


Attachments
the patch (5.31 KB, patch)
2011-07-27 19:37 UTC, Lennart Poettering
Details | Splinter Review
updated patch (5.31 KB, patch)
2011-08-24 14:50 UTC, Lennart Poettering
Details | Splinter Review
updated patch, second try (5.31 KB, patch)
2011-08-24 14:55 UTC, Lennart Poettering
Details | Splinter Review
Third try (5.22 KB, patch)
2011-08-24 14:56 UTC, Lennart Poettering
Details | Splinter Review
patch dropping stdlib.h inclusion (5.18 KB, patch)
2011-08-24 15:04 UTC, Lennart Poettering
Details | Splinter Review
newest patch version, this time adding $(SYSTEMD_LIBS) in Makefile.am (5.76 KB, patch)
2011-08-26 15:08 UTC, Lennart Poettering
Details | Splinter Review
Rebased to current git (5.53 KB, patch)
2012-02-01 20:07 UTC, Lennart Poettering
Details | Splinter Review

Description Lennart Poettering 2011-07-27 19:37:15 UTC
Created attachment 49635 [details] [review]
the patch

systemd nowadays manages seats, users and sessions. Hence call into systemd when we need this information. For details see patch.
Comment 1 Simon McVittie 2011-07-28 07:50:58 UTC
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.
Comment 2 Simon McVittie 2011-07-28 07:55:40 UTC
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
Comment 3 Simon McVittie 2011-07-28 08:38:29 UTC
(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.
Comment 4 Lennart Poettering 2011-08-24 14:50:49 UTC
Created attachment 50548 [details] [review]
updated patch

Updated version fixing all issues pointed out.
Comment 5 Lennart Poettering 2011-08-24 14:52:49 UTC
(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...)
Comment 6 Lennart Poettering 2011-08-24 14:55:35 UTC
Created attachment 50549 [details] [review]
updated patch, second try

Apparently I uploaded the wrong patch... Here's another try.
Comment 7 Lennart Poettering 2011-08-24 14:56:55 UTC
Created attachment 50550 [details] [review]
Third try

I am an idiot. But this one should be the right patch.
Comment 8 Lennart Poettering 2011-08-24 15:02:29 UTC
(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.
Comment 9 Lennart Poettering 2011-08-24 15:04:07 UTC
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().
Comment 10 Colin Walters 2011-08-25 18:47:06 UTC
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.
Comment 11 Simon McVittie 2011-08-26 01:50:45 UTC
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.)
Comment 12 Lennart Poettering 2011-08-26 15:07:48 UTC
(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.
Comment 13 Lennart Poettering 2011-08-26 15:08:30 UTC
Created attachment 50606 [details] [review]
newest patch version, this time adding $(SYSTEMD_LIBS) in Makefile.am
Comment 14 Lennart Poettering 2012-02-01 20:07:59 UTC
Created attachment 56491 [details] [review]
Rebased to current git
Comment 15 Simon McVittie 2012-02-07 08:07:18 UTC
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.
Comment 16 Lennart Poettering 2012-02-09 10:42:26 UTC
(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!


Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.