Port to libsecret, #239 #351

NP-Hardass · 2020-08-03T06:12:31Z

Adds libsecret support. I'll leave it up to you guys to decide at a future time about dropping libgnome-keyring support. This should close out Issue #239.

I wasn't able to runtime test the code in-situ, but copied it to a standalone file and it locked the databased as expected.

NP-Hardass · 2020-08-03T13:30:56Z

@raveit65 Would you also like a commit removing old, deprecated libgnome-keyring? Or do you want to keep it for the time being?

raveit65 · 2020-08-04T10:34:39Z

@NP-Hardass
Still wondering why there isn't an effort in fedora to drop libgnome-keyring.
But fedora ship libsecret since a long time.
Did you check the situation in other linux/bsd distros?

sc0w · 2020-08-04T11:57:30Z

@NP-Hardass I think you can remove --without-keyring in build.yml

I mean

- scan-build $CHECKERS ./configure --enable-compile-warnings=maximum

instead

  - if [ ${DISTRO_NAME} == "debian" -o ${DISTRO_NAME} == "ubuntu" ];then
  -     scan-build $CHECKERS ./configure --without-keyring --enable-compile-warnings=maximum
  - else
  -     scan-build $CHECKERS ./configure --enable-compile-warnings=maximum
  - fi

sc0w · 2020-08-04T12:02:11Z

we need deprecated libgnome-keyring with this change? it can be removed?

NP-Hardass · 2020-08-04T17:24:33Z

@NP-Hardass
Still wondering why there isn't an effort in fedora to drop libgnome-keyring.
But fedora ship libsecret since a long time.
Did you check the situation in other linux/bsd distros?

Of every pkg listed at v1.24.x on repology, every distro has libsecret support except Linux Mint.

we need deprecated libgnome-keyring with this change? it can be removed?

As is, libgnome-keyring is available if someone enables it, but off by default.

lukefromdc

Just build and installed a rebase of this against today's master, with the very old libgnome-keyring 1.12 finally removed from my system. Note that that package is NOT offered in Debian Unstable at this time. Didn't realize a rebuild without libgnome-keyring would have removed support for/dependency on it and let the package function.

Anyway, this build runs normally and suspending works fine.

Got these runtime warnings, unsure if they have anything to do with this PR, one appears to relate to systemd and the other to key mappings:

(mate-power-manager:218930): PowerManager-WARNING **: 01:58:35.670: Failed to get session for pid 218930: The name org.freedesktop.ConsoleKit was not provided by any .service files

(mate-power-manager:218930): PowerManager-WARNING **: 01:58:35.682: could not map keysym 1008ffa8 to keycode

sc0w · 2020-08-07T21:12:23Z

@NP-Hardass

Of every pkg listed at v1.24.x on repology, every distro has libsecret support except Linux Mint.

Linux Mint is based on ubuntu and debian, so, I suppose it has libsecret

lukefromdc · 2020-08-07T21:27:52Z

Libsecret I have, it's the old deprecated libgnome-keyring that is no longer in Sid. This is one of the reasons we need this PR

raveit65 · 2020-09-03T11:33:32Z

@NP-Hardass
Can you please rebase with master and update PR for meson?
#338 is merged now

NP-Hardass · 2020-09-03T13:50:27Z

I can certainly try. I don't have any meson experience though.

…

On Thu, Sep 3, 2020, 7:33 AM raveit65 ***@***.***> wrote: @NP-Hardass <https://github.com/NP-Hardass> Can you please rebase with master and update PR for meson? #338 <#338> is merged now — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#351 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AA6E2P5UKCBQQYVHPBM5UWDSD55JVANCNFSM4PS73BQA> .

NP-Hardass · 2020-09-04T21:19:31Z

If someone could verify it works properly, as I don't have meson configured atm...

lukefromdc

Autotools build worked fine on desktop, though note that desktops support only limited features on mate-power-manager.

Meson build with

meson build/ --prefix /usr
ninja -C build

completed fine.
Installation with
sudo ninja -C build install
stalled temporarily twice but eventually restarted and appeared to finish and return.
This meson build once installed did not run, generated these warnings

** (mate-power-manager:8937): WARNING **: 18:20:10.435: Failed to get session for pid 8937: The name org.freedesktop.ConsoleKit was not provided by any .service files

** (mate-power-manager:8937): WARNING **: 18:20:11.252: could not map keysym 1008ffa8 to keycode

** (mate-power-manager:8937): ERROR **: 18:20:11.259: failed to get value: Failed to execute child process “/usr/sbin/mate-power-backlight-helper” (No such file or directory)
Trace/breakpoint trap
luke@ubuntu:~/Desktop/Development/MATE_Development_Work/mate-power-manager_test-9-4-2020$

The third warning is the one causing the crash and is happened only in the meson build and not the autotools build.
Looks like /usr/sbin/mate-power-backlight-helper either was not installed or was installed to the wrong place and not found

On Uninstall:
install finished.

Deleted: 571
Failed: 0

Remember that files created by custom scripts have not been removed.

raveit65 · 2020-09-05T07:20:38Z

If someone could verify it works properly, as I don't have meson configured atm...

That's easy:

meson build -Dprefix=/usr (w/h -Dprefix it's /usr/local/)
ninja -C build
sudo ninja -C build install

dist:
meson dist -C build

raveit65 · 2020-09-05T07:52:03Z

There is a conflict with master. You should do a git rebase master.

raveit65 · 2020-09-05T08:07:12Z

@NP-Hardass @lukefromdc
The segfault is caused by meson PR. Installing master branch with meson give me same error.

[rave@mother ~]$ /usr/bin/mate-power-manager 

** (mate-power-manager:6737): WARNING **: 09:59:52.272: Failed to get session for pid 6737: The name is not activatable

** (mate-power-manager:6737): WARNING **: 09:59:52.277: could not map keysym 1008ffa8 to keycode

** (mate-power-manager:6737): ERROR **: 09:59:52.282: failed to get value: Failed to execute child process ?/usr/sbin/mate-power-backlight-helper? (No such file or directory)
Trace/breakpoint trap (core dumped)

The other warnings we get with using autotools too. They are not new.

lukefromdc · 2020-09-05T20:42:38Z

Just posted my results to #338 (comment) and yes, I agree we should revert the Meson PR, merge this, fix the Meson stuff, then merge it again.

I agree that master should not be allowed to remain broken at any time

raveit65 · 2020-09-10T08:17:23Z

@NP-Hardass
Meson PR is reverted. Can you please remove meson commit from your PR.
Then this can be tested.

NP-Hardass · 2020-09-11T14:15:08Z

@NP-Hardass
Meson PR is reverted. Can you please remove meson commit from your PR.
Then this can be tested.

Done

src/gpm-control.c

raveit65 · 2020-09-12T16:53:23Z

@NP-Hardass
Normally our code style is not to use tabs i newly written code, but whole gpm-control.c file use tab=4 size, so you should do the same.

raveit65 · 2020-10-03T09:34:59Z

@NP-Hardass
Code-formatting-style needs to be fixed.

raveit65 · 2020-10-13T09:37:19Z

Don't think i like to use tabs in code but they are used all over this file, so we should respect it.
https://www.dropbox.com/s/bxnwf3k38pr6uoa/0001-gpm-control.c-Add-libsecret-implementation-to-gpm_co.patch?dl=0

From 86b1025e9708ad0913084f33a8421bdfb93ff682 Mon Sep 17 00:00:00 2001
From: NP-Hardass <[email protected]>
Date: Mon, 3 Aug 2020 01:49:47 -0400
Subject: [PATCH 1/5] gpm-control.c: Add libsecret implementation to
 gpm_control_suspend()

---
 src/gpm-control.c | 40 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 40 insertions(+)

diff --git a/src/gpm-control.c b/src/gpm-control.c
index 4834a05..ef0b385 100644
--- a/src/gpm-control.c
+++ b/src/gpm-control.c
@@ -39,6 +39,9 @@
 #include <gio/gio.h>
 #include <glib/gi18n.h>
 
+#ifdef WITH_LIBSECRET
+#include <libsecret/secret.h>
+#endif /* WITH_LIBSECRET */
 #ifdef WITH_KEYRING
 #include <gnome-keyring.h>
 #endif /* WITH_KEYRING */
@@ -210,6 +213,13 @@ gpm_control_suspend (GpmControl *control, GError **error)
 	EggConsoleKit *console;
 	GpmScreensaver *screensaver;
 	guint32 throttle_cookie = 0;
+#ifdef WITH_LIBSECRET
+	gboolean lock_libsecret;
+	GCancellable *libsecret_cancellable = NULL;
+	SecretService *secretservice_proxy = NULL;
+	gint num_secrets_locked;
+	GList *libsecret_collections = NULL;
+#endif /* WITH_LIBSECRET */
 #ifdef WITH_KEYRING
 	gboolean lock_gnome_keyring;
 	GnomeKeyringResult keyres;
@@ -233,6 +243,36 @@ gpm_control_suspend (GpmControl *control, GError **error)
 		}
 	}
 
+#ifdef WITH_LIBSECRET
+	/* we should perhaps lock keyrings when sleeping #375681 */
+	lock_libsecret = g_settings_get_boolean (control->priv->settings,
+						 GPM_SETTINGS_LOCK_KEYRING_SUSPEND);
+	if (lock_libsecret) {
+		libsecret_cancellable = g_cancellable_new ();
+		secretservice_proxy = secret_service_get_sync (SECRET_SERVICE_LOAD_COLLECTIONS,
+							       libsecret_cancellable,
+							       error);
+		if (secretservice_proxy == NULL) {
+			g_warning ("failed to connect to secret service");
+		} else {
+			libsecret_collections = secret_service_get_collections (secretservice_proxy);
+			if ( libsecret_collections == NULL) {
+				g_warning ("failed to get secret collections");
+			} else {
+				num_secrets_locked = secret_service_lock_sync (secretservice_proxy,
+									       libsecret_collections,
+									       libsecret_cancellable,
+									       NULL,
+									       error);
+				if (num_secrets_locked <= 0)
+					g_warning ("could not lock keyring");
+				g_list_free (libsecret_collections);
+			}
+	 		g_object_unref (secretservice_proxy);
+		}
+		g_object_unref (libsecret_cancellable);
+	}
+#endif /* WITH_LIBSECRET */
 #ifdef WITH_KEYRING
 	/* we should perhaps lock keyrings when sleeping #375681 */
 	lock_gnome_keyring = g_settings_get_boolean (control->priv->settings, GPM_SETTINGS_LOCK_KEYRING_SUSPEND);
-- 
2.26.2

https://www.dropbox.com/s/y7wdu5h9f5k5qil/0002-gpm-control.c-Add-libsecret-implementation-to-gpm_co.patch?dl=0

From ffa759299652683bb95dcc939546060360a5a0d7 Mon Sep 17 00:00:00 2001
From: NP-Hardass <[email protected]>
Date: Mon, 3 Aug 2020 01:55:43 -0400
Subject: [PATCH 2/5] gpm-control.c: Add libsecret implementation to
 gpm_control_hibernate()

---
 src/gpm-control.c | 37 +++++++++++++++++++++++++++++++++++++
 1 file changed, 37 insertions(+)

diff --git a/src/gpm-control.c b/src/gpm-control.c
index ef0b385..c1109db 100644
--- a/src/gpm-control.c
+++ b/src/gpm-control.c
@@ -368,6 +368,13 @@ gpm_control_hibernate (GpmControl *control, GError **error)
 	EggConsoleKit *console;
 	GpmScreensaver *screensaver;
 	guint32 throttle_cookie = 0;
+#ifdef WITH_LIBSECRET
+	gboolean lock_libsecret;
+	GCancellable *libsecret_cancellable = NULL;
+	SecretService *secretservice_proxy = NULL;
+	gint num_secrets_locked;
+	GList *libsecret_collections = NULL;
+#endif /* WITH_LIBSECRET */
 #ifdef WITH_KEYRING
 	gboolean lock_gnome_keyring;
 	GnomeKeyringResult keyres;
@@ -391,6 +398,36 @@ gpm_control_hibernate (GpmControl *control, GError **error)
 		}
 	}
 
+#ifdef WITH_LIBSECRET
+	/* we should perhaps lock keyrings when sleeping #375681 */
+	lock_libsecret = g_settings_get_boolean (control->priv->settings,
+						 GPM_SETTINGS_LOCK_KEYRING_SUSPEND);
+	if (lock_libsecret) {
+		libsecret_cancellable = g_cancellable_new ();
+		secretservice_proxy = secret_service_get_sync (SECRET_SERVICE_LOAD_COLLECTIONS,
+							       libsecret_cancellable,
+							       error);
+		if (secretservice_proxy == NULL) {
+			g_warning ("failed to connect to secret service");
+		} else {
+			libsecret_collections = secret_service_get_collections (secretservice_proxy);
+			if (libsecret_collections == NULL) {
+				g_warning ("failed to get secret collections");
+			} else {
+				num_secrets_locked = secret_service_lock_sync (secretservice_proxy,
+									       libsecret_collections,
+									       libsecret_cancellable,
+									       NULL,
+									       error);
+				if (num_secrets_locked <= 0)
+					g_warning ("could not lock keyring");
+				g_list_free (libsecret_collections);
+			}
+			g_object_unref (secretservice_proxy);
+		}
+		g_object_unref (libsecret_cancellable);
+	}
+#endif /* WITH_LIBSECRET */
 #ifdef WITH_KEYRING
 	/* we should perhaps lock keyrings when sleeping #375681 */
 	lock_gnome_keyring = g_settings_get_boolean (control->priv->settings, GPM_SETTINGS_LOCK_KEYRING_HIBERNATE);
-- 
2.26.2

Lines are shorter as in other parts of this file.
Commits use correct indents level
A space is added before opening brackets

Does this looks better, or not?

NP-Hardass · 2020-10-13T17:01:16Z

Don't think i like to use tabs in code but they are used all over this file, so we should respect it.

Lines are shorter as in other parts of this file.

Commits use correct indents level

A space is added before opening brackets

Does this looks better, or not?

+#ifdef WITH_LIBSECRET
+	/* we should perhaps lock keyrings when sleeping #375681 */
+	lock_libsecret = g_settings_get_boolean (control->priv->settings,
+						 GPM_SETTINGS_LOCK_KEYRING_SUSPEND);

Updated accordingly. The only change you made that stuck out to me was the above quoted, because it changes a line that existed for KEYRING to wrap. I have no feelings one way or the other though.

src/gpm-control.c

raveit65

LGTM,
thanks.

raveit65 · 2020-10-13T21:17:48Z

@mate-desktop/core-team
Any other reviewer who like to review this PR?

lukefromdc · 2020-10-21T20:20:41Z

Builds and runs fine on my desktop with libsecret support enabled, same warnings I've always gotten on starting from terminal

(mate-power-manager:506313): PowerManager-WARNING **: 16:16:23.995: Failed to get session for pid 506313: The name org.freedesktop.ConsoleKit was not provided by any .service files

(mate-power-manager:506313): PowerManager-WARNING **: 16:16:24.001: could not map keysym 1008ffa8 to keycode

(mate-power-manager:506313): Gdk-CRITICAL **: 16:16:24.088: gdk_window_thaw_toplevel_updates: assertion 'window->update_and_descendants_freeze_count > 0' failed

(mate-power-preferences:506350): PowerManager-WARNING **: 16:16:29.373: Failed to get session for pid 506350: The name org.freedesktop.ConsoleKit was not provided by any .service files

Nothing in the warnings seems to come from this change.

NP-Hardass force-pushed the port-to-libsecret branch from 40112b8 to 4935363 Compare August 3, 2020 13:18

NP-Hardass force-pushed the port-to-libsecret branch from 4935363 to a5b4e5b Compare August 3, 2020 13:40

raveit65 requested review from raveit65, rbuj, monsta, clefebvre, cwendling, flexiondotorg, lukefromdc, sc0w, vkareh, yetist and a team August 4, 2020 10:35

lukefromdc reviewed Aug 5, 2020

View reviewed changes

raveit65 requested a review from a team September 3, 2020 11:34

NP-Hardass force-pushed the port-to-libsecret branch from a5b4e5b to 6e83c51 Compare September 4, 2020 20:50

lukefromdc reviewed Sep 4, 2020

View reviewed changes

Add autotools support for libsecret

33e806d

NP-Hardass force-pushed the port-to-libsecret branch from 6579d33 to 16138ac Compare September 10, 2020 21:57

raveit65 reviewed Sep 12, 2020

View reviewed changes