Port to libsecret, #239

[NP-Hardass]

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]

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

[raveit65]

@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]

@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]

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

[NP-Hardass]

@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.

[sc0w]

@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]

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]

@NP-Hardass Can you please rebase with master and update PR for meson? https://github.com/mate-desktop/mate-power-manager/pull/338 is merged now

[NP-Hardass]

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

On Thu, Sep 3, 2020, 7:33 AM raveit65 notifications@github.com wrote:

@NP-Hardass https://github.com/NP-Hardass Can you please rebase with master and update PR for meson?

338 https://github.com/mate-desktop/mate-power-manager/pull/338 is

merged now

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/mate-desktop/mate-power-manager/pull/351#issuecomment-686428127, or unsubscribe https://github.com/notifications/unsubscribe-auth/AA6E2P5UKCBQQYVHPBM5UWDSD55JVANCNFSM4PS73BQA .

[NP-Hardass]

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

[raveit65]

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]

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

[raveit65]

@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.

[NP-Hardass]

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

Done

[lukefromdc]

Just ran into another Meson-only problem: the inhibit applet and brightness applet won't load, again probably both of them installed to the wrong place

[lukefromdc]

On further testing I found the applets refusing to load on Meson buildfs occurs with master as well as with this PR, but I just got mate-power-manager running with a meson build from master. Thus we have one Meson problem (the applets) that needs to be fixed in master later, and one (the crash) that needs to be fixed here(or maybe that too is in master but giving inconsistant results?).

Also note that my tests were with a local rebase against master

[raveit65]

@lukefromdc Can you please post all that in meson PR? In my opinion this should all fixed by author of meson PR who didn't test his work..... Sorry, i don't know to fix this meson issues. We can also revert this meson stuff and reopen meson PR for fixing all this issues. This would make the way free for porting m-p-m to libsecret. I don't like to have a broken master branch.... @rbuj @vkareh @mate-desktop/core-team What do you think?

[lukefromdc]

Just posted my results to https://github.com/mate-desktop/mate-power-manager/pull/338#issuecomment-687660317 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]

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

[NP-Hardass]

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

Done

[raveit65]

@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]

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

[raveit65]

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 <np.hardass@gmail.com>
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 <np.hardass@gmail.com>
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]

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.

[raveit65]

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

[lukefromdc]

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.