URL: https://github.com/SSSD/sssd/pull/30 Author: pbrezina Title: #30: sssctl: use systemd D-Bus API Action: opened
PR body: """ If systemd is used we leverage it's D-Bus API instead of running systemctl.
Resolves: https://fedorahosted.org/sssd/ticket/3056 """
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/30/head:pr30 git checkout pr30
On Wed, Sep 21, 2016 at 12:27:16PM +0200, pbrezina wrote:
URL: https://github.com/SSSD/sssd/pull/30 Author: pbrezina Title: #30: sssctl: use systemd D-Bus API Action: opened
PR body: """ If systemd is used we leverage it's D-Bus API instead of running systemctl.
Resolves: https://fedorahosted.org/sssd/ticket/3056 """
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/30/head:pr30 git checkout pr30
From 73b6071341262a298e3caf6857361504c8512d6c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pavel=20B=C5=99ezina?= pbrezina@redhat.com Date: Wed, 21 Sep 2016 12:25:43 +0200 Subject: [PATCH] sssctl: use systemd D-Bus API
If systemd is used we leverage it's D-Bus API instead of running systemctl.
Resolves: https://fedorahosted.org/sssd/ticket/3056
Makefile.am | 1 + src/tools/sssctl/sssctl.c | 23 +++++++++++++---------- src/tools/sssctl/sssctl.h | 4 ++++ 3 files changed, 18 insertions(+), 10 deletions(-)
diff --git a/Makefile.am b/Makefile.am index f792ed6..7e6bf7e 100644 --- a/Makefile.am +++ b/Makefile.am @@ -1570,6 +1570,7 @@ sss_override_CFLAGS = \
sssctl_SOURCES = \ src/tools/sssctl/sssctl.c \
- src/tools/sssctl/sssctl_systemd.c \
Did you forget to git add this file?
src/tools/sssctl/sssctl_cache.c \ src/tools/sssctl/sssctl_data.c \ src/tools/sssctl/sssctl_logs.c \
On 09/21/2016 12:31 PM, Jakub Hrozek wrote:
On Wed, Sep 21, 2016 at 12:27:16PM +0200, pbrezina wrote:
URL: https://github.com/SSSD/sssd/pull/30 Author: pbrezina Title: #30: sssctl: use systemd D-Bus API Action: opened
PR body: """ If systemd is used we leverage it's D-Bus API instead of running systemctl.
Resolves: https://fedorahosted.org/sssd/ticket/3056 """
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/30/head:pr30 git checkout pr30
From 73b6071341262a298e3caf6857361504c8512d6c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pavel=20B=C5=99ezina?= pbrezina@redhat.com Date: Wed, 21 Sep 2016 12:25:43 +0200 Subject: [PATCH] sssctl: use systemd D-Bus API
If systemd is used we leverage it's D-Bus API instead of running systemctl.
Resolves: https://fedorahosted.org/sssd/ticket/3056
Makefile.am | 1 + src/tools/sssctl/sssctl.c | 23 +++++++++++++---------- src/tools/sssctl/sssctl.h | 4 ++++ 3 files changed, 18 insertions(+), 10 deletions(-)
diff --git a/Makefile.am b/Makefile.am index f792ed6..7e6bf7e 100644 --- a/Makefile.am +++ b/Makefile.am @@ -1570,6 +1570,7 @@ sss_override_CFLAGS = \
sssctl_SOURCES = \ src/tools/sssctl/sssctl.c \
- src/tools/sssctl/sssctl_systemd.c \
Did you forget to git add this file?
Yes, I pushed it fixed.
URL: https://github.com/SSSD/sssd/pull/30 Title: #30: sssctl: use systemd D-Bus API
Label: +Changes requested
URL: https://github.com/SSSD/sssd/pull/30 Author: pbrezina Title: #30: sssctl: use systemd D-Bus API Action: synchronized
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/30/head:pr30 git checkout pr30
URL: https://github.com/SSSD/sssd/pull/30 Title: #30: sssctl: use systemd D-Bus API
Label: -Changes requested
URL: https://github.com/SSSD/sssd/pull/30 Title: #30: sssctl: use systemd D-Bus API
jhrozek commented: """ On Wed, Sep 21, 2016 at 03:27:13AM -0700, Pavel Březina wrote:
If systemd is used we leverage it's D-Bus API instead of running systemctl.
Resolves: https://fedorahosted.org/sssd/ticket/3056 You can view, comment on, or merge this pull request online at:
https://github.com/SSSD/sssd/pull/30
-- Commit Summary --
- sssctl: use systemd D-Bus API
-- File Changes --
M Makefile.am (1) M src/tools/sssctl/sssctl.c (23) M src/tools/sssctl/sssctl.h (4)-- Patch Links --
https://github.com/SSSD/sssd/pull/30.patch https://github.com/SSSD/sssd/pull/30.diff
So far I just scrolled through the diff and it looks good to me. Can you please add some DEBUG messages for both failure cases and some trace messages so we can follow the code flow?
Additionally, should we call /sbin/service instead of service so that we don't rely on $PATH?
"""
See the full comment at https://github.com/SSSD/sssd/pull/30#issuecomment-249141263
URL: https://github.com/SSSD/sssd/pull/30 Title: #30: sssctl: use systemd D-Bus API
Label: +Changes requested
URL: https://github.com/SSSD/sssd/pull/30 Author: pbrezina Title: #30: sssctl: use systemd D-Bus API Action: synchronized
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/30/head:pr30 git checkout pr30
URL: https://github.com/SSSD/sssd/pull/30 Title: #30: sssctl: use systemd D-Bus API
pbrezina commented: """ I added debug messages and attached a new patch for /sbin/service. I wonder if we should detect the path with configure? """
See the full comment at https://github.com/SSSD/sssd/pull/30#issuecomment-249152704
URL: https://github.com/SSSD/sssd/pull/30 Title: #30: sssctl: use systemd D-Bus API
Label: -Changes requested
URL: https://github.com/SSSD/sssd/pull/30 Title: #30: sssctl: use systemd D-Bus API
jhrozek commented: """ Looks like there are some warnings: {{{ Error: UNINIT (CWE-457): sssd-1.14.2/src/tools/sssctl/sssctl_systemd.c:60: var_decl: Declaring variable "msg" without initializer. sssd-1.14.2/src/tools/sssctl/sssctl_systemd.c:102: uninit_use: Using uninitialized value "msg". # 100| # 101| done: # 102|-> if (msg != NULL) { # 103| dbus_message_unref(msg); # 104| }
Error: COMPILER_WARNING: sssd-1.14.2/src/tools/sssctl/sssctl_systemd.c: scope_hint: In function 'sssctl_systemd_call' sssd-1.14.2/src/tools/sssctl/sssctl_systemd.c:102:8: warning: 'msg' may be used uninitialized in this function [-Wmaybe-uninitialized] # if (msg != NULL) { # ^ # 100| # 101| done: # 102|-> if (msg != NULL) { # 103| dbus_message_unref(msg); # 104| }
Error: UNINIT (CWE-457): sssd-1.14.2/src/tools/sssctl/sssctl_systemd.c:59: var_decl: Declaring variable "reply" without initializer. sssd-1.14.2/src/tools/sssctl/sssctl_systemd.c:106: uninit_use: Using uninitialized value "reply". # 104| } # 105| # 106|-> if (reply != NULL) { # 107| dbus_message_unref(reply); # 108| }
Error: COMPILER_WARNING: sssd-1.14.2/src/tools/sssctl/sssctl_systemd.c:106:8: warning: 'reply' may be used uninitialized in this function [-Wmaybe-uninitialized] # if (reply != NULL) { # ^ # 104| } # 105| # 106|-> if (reply != NULL) { # 107| dbus_message_unref(reply); # 108| } }}} """
See the full comment at https://github.com/SSSD/sssd/pull/30#issuecomment-249536130
URL: https://github.com/SSSD/sssd/pull/30 Title: #30: sssctl: use systemd D-Bus API
Label: +Changes requested
URL: https://github.com/SSSD/sssd/pull/30 Title: #30: sssctl: use systemd D-Bus API
jhrozek commented: """ Looks like there are some warnings:
``` Error: UNINIT (CWE-457): sssd-1.14.2/src/tools/sssctl/sssctl_systemd.c:60: var_decl: Declaring variable "msg" without initializer. sssd-1.14.2/src/tools/sssctl/sssctl_systemd.c:102: uninit_use: Using uninitialized value "msg". # 100| # 101| done: # 102|-> if (msg != NULL) { # 103| dbus_message_unref(msg); # 104| }
Error: COMPILER_WARNING: sssd-1.14.2/src/tools/sssctl/sssctl_systemd.c: scope_hint: In function 'sssctl_systemd_call' sssd-1.14.2/src/tools/sssctl/sssctl_systemd.c:102:8: warning: 'msg' may be used uninitialized in this function [-Wmaybe-uninitialized] # if (msg != NULL) { # ^ # 100| # 101| done: # 102|-> if (msg != NULL) { # 103| dbus_message_unref(msg); # 104| }
Error: UNINIT (CWE-457): sssd-1.14.2/src/tools/sssctl/sssctl_systemd.c:59: var_decl: Declaring variable "reply" without initializer. sssd-1.14.2/src/tools/sssctl/sssctl_systemd.c:106: uninit_use: Using uninitialized value "reply". # 104| } # 105| # 106|-> if (reply != NULL) { # 107| dbus_message_unref(reply); # 108| }
Error: COMPILER_WARNING: sssd-1.14.2/src/tools/sssctl/sssctl_systemd.c:106:8: warning: 'reply' may be used uninitialized in this function [-Wmaybe-uninitialized] # if (reply != NULL) { # ^ # 104| } # 105| # 106|-> if (reply != NULL) { # 107| dbus_message_unref(reply); # 108| } ``` """
See the full comment at https://github.com/SSSD/sssd/pull/30#issuecomment-249536130
URL: https://github.com/SSSD/sssd/pull/30 Title: #30: sssctl: use systemd D-Bus API
jhrozek commented: """ Hey @pbrezina just making sure you didn't miss the review. About /sbin/service...since there are fewer and fewer non-systemd systemd, I wouldn't bother making this configurable unless someone asks. I'll just try to run a simple test on Debian/Ubuntu w/o systemd to make sure this path works there as well.. """
See the full comment at https://github.com/SSSD/sssd/pull/30#issuecomment-251485499
URL: https://github.com/SSSD/sssd/pull/30 Title: #30: sssctl: use systemd D-Bus API
jhrozek commented: """ Hey @pbrezina just making sure you didn't miss the review. About /sbin/service...since there are fewer and fewer non-systemd systems, I wouldn't bother making this configurable unless someone asks. I'll just try to run a simple test on Debian/Ubuntu w/o systemd to make sure this path works there as well.. """
See the full comment at https://github.com/SSSD/sssd/pull/30#issuecomment-251485499
URL: https://github.com/SSSD/sssd/pull/30 Author: pbrezina Title: #30: sssctl: use systemd D-Bus API Action: synchronized
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/30/head:pr30 git checkout pr30
URL: https://github.com/SSSD/sssd/pull/30 Title: #30: sssctl: use systemd D-Bus API
pbrezina commented: """ Updated version was pushed. I initialized D-Bus variables in sssctl_systmed_call:
static errno_t sssctl_systemd_call(const char *method) { DBusConnection *conn = NULL; DBusMessage *reply = NULL; DBusMessage *msg = NULL; DBusError error; const char *unit = SSS_SYSTEMD_UNIT; const char *mode = SSS_SYSTEMD_MODE; const char *job; errno_t ret; """
See the full comment at https://github.com/SSSD/sssd/pull/30#issuecomment-251656376
URL: https://github.com/SSSD/sssd/pull/30 Title: #30: sssctl: use systemd D-Bus API
pbrezina commented: """ Updated version was pushed. I initialized D-Bus variables in sssctl_systmed_call:
```c static errno_t sssctl_systemd_call(const char *method) { DBusConnection *conn = NULL; DBusMessage *reply = NULL; DBusMessage *msg = NULL; DBusError error; const char *unit = SSS_SYSTEMD_UNIT; const char *mode = SSS_SYSTEMD_MODE; const char *job; errno_t ret; ``` """
See the full comment at https://github.com/SSSD/sssd/pull/30#issuecomment-251656376
URL: https://github.com/SSSD/sssd/pull/30 Title: #30: sssctl: use systemd D-Bus API
pbrezina commented: """ Updated version was pushed. I initialized D-Bus variables in sssctl_systmed_call:
```c static errno_t sssctl_systemd_call(const char *method) { DBusConnection *conn = NULL; DBusMessage *reply = NULL; DBusMessage *msg = NULL; DBusError error; const char *unit = SSS_SYSTEMD_UNIT; const char *mode = SSS_SYSTEMD_MODE; const char *job; errno_t ret; ``` """
See the full comment at https://github.com/SSSD/sssd/pull/30#issuecomment-251656376
URL: https://github.com/SSSD/sssd/pull/30 Title: #30: sssctl: use systemd D-Bus API
pbrezina commented: """ Updated version was pushed. I initialized D-Bus variables in `sssctl_systemd_call()`:
```c static errno_t sssctl_systemd_call(const char *method) { DBusConnection *conn = NULL; DBusMessage *reply = NULL; DBusMessage *msg = NULL; DBusError error; const char *unit = SSS_SYSTEMD_UNIT; const char *mode = SSS_SYSTEMD_MODE; const char *job; errno_t ret; ``` """
See the full comment at https://github.com/SSSD/sssd/pull/30#issuecomment-251656376
URL: https://github.com/SSSD/sssd/pull/30 Title: #30: sssctl: use systemd D-Bus API
Label: -Changes requested
URL: https://github.com/SSSD/sssd/pull/30 Title: #30: sssctl: use systemd D-Bus API
jhrozek commented: """ Thanks, this looks good, but I wonder if we could really just detect the service command (I know we talked about it and then decided not to..but I forgot why).
See for example: https://github.com/jhrozek/sssd/tree/service_restart """
See the full comment at https://github.com/SSSD/sssd/pull/30#issuecomment-252574427
URL: https://github.com/SSSD/sssd/pull/30 Title: #30: sssctl: use systemd D-Bus API
Label: +Changes requested
URL: https://github.com/SSSD/sssd/pull/30 Title: #30: sssctl: use systemd D-Bus API
pbrezina commented: """ Hi, your patch looks good to me. There was no specific reason, see https://github.com/SSSD/sssd/pull/30#issuecomment-251485499 """
See the full comment at https://github.com/SSSD/sssd/pull/30#issuecomment-252577813
URL: https://github.com/SSSD/sssd/pull/30 Title: #30: sssctl: use systemd D-Bus API
jhrozek commented: """ On Mon, Oct 10, 2016 at 03:04:35AM -0700, Pavel Březina wrote:
Hi, your patch looks good to me. There was no specific reason, see https://github.com/SSSD/sssd/pull/30#issuecomment-251485499
So if you agree, I will push the patchset with my RB for your patches and your RB for my patch.
(And yes, sorry, I knew we talked about this in the past, I just disliked the non-absolute patch when I saw it in the code..which is also why I wanted to write the patch myself and not to ask you to do X after asking you to do Y before..)
"""
See the full comment at https://github.com/SSSD/sssd/pull/30#issuecomment-252871228
URL: https://github.com/SSSD/sssd/pull/30 Title: #30: sssctl: use systemd D-Bus API
Label: -Changes requested
URL: https://github.com/SSSD/sssd/pull/30 Title: #30: sssctl: use systemd D-Bus API
Label: +Accepted
URL: https://github.com/SSSD/sssd/pull/30 Title: #30: sssctl: use systemd D-Bus API
pbrezina commented: """ No problem. Feel free to push it. """
See the full comment at https://github.com/SSSD/sssd/pull/30#issuecomment-252873755
URL: https://github.com/SSSD/sssd/pull/30 Title: #30: sssctl: use systemd D-Bus API
Label: +Pushed
URL: https://github.com/SSSD/sssd/pull/30 Title: #30: sssctl: use systemd D-Bus API
Label: -Accepted
URL: https://github.com/SSSD/sssd/pull/30 Title: #30: sssctl: use systemd D-Bus API
jhrozek commented: """ * master: 03713a6..761515e """
See the full comment at https://github.com/SSSD/sssd/pull/30#issuecomment-252905006
URL: https://github.com/SSSD/sssd/pull/30 Author: pbrezina Title: #30: sssctl: use systemd D-Bus API Action: closed
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/30/head:pr30 git checkout pr30
sssd-devel@lists.fedorahosted.org
-
fidencio -
Jakub Hrozek -
Pavel Březina