Bug 85242 - Add critical action support for !Linux
Summary: Add critical action support for !Linux
Status: RESOLVED FIXED
Alias: None
Product: upower
Classification: Unclassified
Component: general (show other bugs)
Version: unspecified
Hardware: Other All
: medium enhancement
Assignee: Richard Hughes
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-10-20 13:19 UTC by Eric Koegel
Modified: 2015-07-07 15:12 UTC (History)
2 users (show)

See Also:
i915 platform:
i915 features:


Attachments
Add critical action support for !Linux (12.30 KB, patch)
2014-10-20 13:19 UTC, Eric Koegel
Details | Splinter Review
Add critical action support for BSD (7.81 KB, patch)
2015-05-11 07:53 UTC, Eric Koegel
Details | Splinter Review
Add critical action support for BSD (14.50 KB, patch)
2015-07-06 09:37 UTC, Eric Koegel
Details | Splinter Review
Add critical action support for BSD (13.51 KB, patch)
2015-07-06 17:43 UTC, Eric Koegel
Details | Splinter Review

Description Eric Koegel 2014-10-20 13:19:54 UTC
Created attachment 108116 [details] [review]
Add critical action support for !Linux

This patch adds support for using ConsoleKit2's DBUS API to
implement the critical action for FreeBSD and OpenBSD (and PC-BSD, DragonFlyBSD, etc). It also implements it on Linux since it's not invasive to the code changes
and may make it easier to test.

If ConsoleKit2 isn't installed the critical action of power off will still work for the BSDs as ConsoleKit2 is backwards compatible with ConsoleKit.

ConsoleKit2 is a fork of ConsoleKit since it is no longer maintained.
https://github.com/ConsoleKit2/ConsoleKit2
Comment 1 Bastien Nocera 2015-04-02 12:49:23 UTC
Comment on attachment 108116 [details] [review]
Add critical action support for !Linux

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

Please remove the Linux support, we don't want to use ConsoleKit2 on Linux.

The code for OpenBSD and FreeBSD looks pretty much the exact same, could you move it into a shared (or at least copy/pasted) file?

Looks good other than that.
Comment 2 Eric Koegel 2015-05-11 07:53:06 UTC
Created attachment 115689 [details] [review]
Add critical action support for BSD

>Please remove the Linux support, we don't want to use ConsoleKit2 on Linux.

Done.

>The code for OpenBSD and FreeBSD looks pretty much the exact same, could you move it into a shared (or at least copy/pasted) file?

Sure thing. I made a unix/up-backend-common.c file both backends use. Since it's in a different location I don't know how to interact with the UpBackendPrivate so I just attached the data needed to the object and let the object's life-cycle management handle calling unref with g_object_set_data_full. This is only for the UpConfig and GDBusProxy.
Comment 3 Bastien Nocera 2015-05-13 10:04:11 UTC
Comment on attachment 115689 [details] [review]
Add critical action support for BSD

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

::: src/Makefile.am
@@ +2,5 @@
>  
>  SUBDIRS = dummy freebsd linux openbsd
>  
> +if BACKEND_TYPE_FREEBSD
> +SUBDIRS += unix

I'd rather it was called "bsd", as I'm pretty sure that FreeBSD didn't pay Open Group to get that tag.

::: src/unix/up-backend-common.c
@@ +61,5 @@
> +								    CONSOLEKIT2_DBUS_INTERFACE,
> +								    NULL,
> +								    NULL);
> +
> +		g_object_set_data_full (G_OBJECT (backend), "seat_manager_proxy", seat_manager_proxy, g_object_unref);

Instead, add a "up-backend-bsd-private.h" header in the same directory, and add a:
up_backend_set_set_manager_proxy();
function in that header, and in the openbsd/freebsd up-backend.c

@@ +83,5 @@
> +	if (temp == NULL || !UP_IS_CONFIG (temp)) {
> +		/* Create and store the config for future use */
> +		config = up_config_new ();
> +
> +		g_object_set_data_full (G_OBJECT (backend), "UpConfig", config, g_object_unref);

Same thing with up_backend_set_config(); (or get_config in each of the up-backend.c?)
Comment 4 Eric Koegel 2015-07-06 09:37:10 UTC
Created attachment 116970 [details] [review]
Add critical action support for BSD

(In reply to Bastien Nocera from comment #3)

> I'd rather it was called "bsd", as I'm pretty sure that FreeBSD didn't pay
> Open Group to get that tag.
>

Changed to bsd


> Same thing with up_backend_set_config(); (or get_config in each of the
> up-backend.c?)

I added the up-backend-bsd-private.h header but changed it so that both calls
are gets for the respective functions and have the backends manage the objects.
Otherwise the private header would need a get and set pair for each. This way
the respective backend handles those. Let me know if you don't like this and
I'll happily change it!

Thanks for taking the time to do these reviews!
Comment 5 Bastien Nocera 2015-07-06 10:03:41 UTC
Comment on attachment 116970 [details] [review]
Add critical action support for BSD

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

::: src/Makefile.am
@@ +2,5 @@
>  
>  SUBDIRS = dummy freebsd linux openbsd
>  
> +if BACKEND_TYPE_FREEBSD
> +SUBDIRS += bsd

You'll need to add a DIST_SUBDIRS with all the sub-directories listed, so that the bsd subdir is included in the tarball created on Linux.

::: src/bsd/Makefile.am
@@ +10,5 @@
> +	$(GLIB_CFLAGS)						\
> +	$(GIO_CFLAGS)
> +
> +
> +if BACKEND_TYPE_FREEBSD

That's not necessary, the subdirectory will only be entered if BACKEND_TYPE_FREEBSD or BACKEND_TYPE_OPENBSD is set.

@@ +14,5 @@
> +if BACKEND_TYPE_FREEBSD
> +noinst_LTLIBRARIES = libupsharedcommon.la
> +endif
> +
> +if BACKEND_TYPE_OPENBSD

Ditto.

::: src/freebsd/up-backend.c
@@ +329,5 @@
>   * @backend: The %UpBackend class instance
>   *
> + * Returns the seat manager object or NULL on error. [transfer none]
> + */
> +GDBusProxy*

Space after the variable type.

@@ +343,5 @@
>   * @backend: The %UpBackend class instance
>   *
> + * Returns the UpConfig object or NULL on error. [transfer none]
> + */
> +UpConfig*

Ditto.

::: src/openbsd/up-backend.c
@@ +40,4 @@
>  static gboolean		up_apm_device_get_online		(UpDevice *device, gboolean *online);
>  static gboolean		up_apm_device_refresh		(UpDevice *device);
>  
> +#define CONSOLEKIT2_DBUS_NAME                  "org.freedesktop.ConsoleKit"

Want to move those defines into up-backend-bsd-private.h?

@@ +176,5 @@
>   * @backend: The %UpBackend class instance
>   *
> + * Returns the seat manager object or NULL on error. [transfer none]
> + */
> +GDBusProxy*

Space.

@@ +190,5 @@
>   * @backend: The %UpBackend class instance
>   *
> + * Returns the UpConfig object or NULL on error. [transfer none]
> + */
> +UpConfig*

Space.
Comment 6 Eric Koegel 2015-07-06 17:43:24 UTC
Created attachment 116983 [details] [review]
Add critical action support for BSD

(In reply to Bastien Nocera from comment #5)
> Comment on attachment 116970 [details] [review] [review]
> Add critical action support for BSD
> 
> Review of attachment 116970 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> ::: src/Makefile.am
> You'll need to add a DIST_SUBDIRS with all the sub-directories listed, so
> that the bsd subdir is included in the tarball created on Linux.

Added the DIST_SUBDIRS

> ::: src/bsd/Makefile.am
> @@ +10,5 @@
> > +	$(GLIB_CFLAGS)						\
> > +	$(GIO_CFLAGS)
> > +
> > +
> > +if BACKEND_TYPE_FREEBSD
> 
> That's not necessary, the subdirectory will only be entered if
> BACKEND_TYPE_FREEBSD or BACKEND_TYPE_OPENBSD is set.

Removed both those checks.

> ::: src/freebsd/up-backend.c
> @@ +329,5 @@
> >   * @backend: The %UpBackend class instance
> >   *
> > + * Returns the seat manager object or NULL on error. [transfer none]
> > + */
> > +GDBusProxy*
> 
> Space after the variable type.

Fixed in all those locations.

> ::: src/openbsd/up-backend.c
> @@ +40,4 @@
> >  static gboolean		up_apm_device_get_online		(UpDevice *device, gboolean *online);
> >  static gboolean		up_apm_device_refresh		(UpDevice *device);
> >  
> > +#define CONSOLEKIT2_DBUS_NAME                  "org.freedesktop.ConsoleKit"
> 
> Want to move those defines into up-backend-bsd-private.h?

Yes. Moved them.

Thanks once more for the quick review.
Comment 7 Bastien Nocera 2015-07-07 15:12:10 UTC
Pushed to master, 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.