[SSSD] Re: [PATCHES] sssctl: print active server and server list

On Thu, Jul 14, 2016 at 11:39:40AM +0200, Pavel Březina wrote:

On 07/13/2016 12:11 PM, Jakub Hrozek wrote:

...

On Thu, Jun 30, 2016 at 02:10:47PM +0200, Pavel Březina wrote:

...

Failover patches for the sssctl tool. The output looks like:

[master.ipa.pb: ~]$ sudo sssctl domain-status ad.pb Online status: Online

Active servers: AD Global Catalog: root-dc.ad.pb AD Domain Controller: root-dc.ad.pb IPA: master.ipa.pb

Discovered AD Global Catalog servers:

• root-dc.ad.pb
• invalid.ad.pb
• root-dc.ad.pb

Discovered AD Domain Controller servers:

• root-dc.ad.pb

Discovered IPA servers:

• master.ipa.pb

This is the best output format I could thing of so far, but I'm opened for suggestions.

I'm sorry for the late review. I started by sending the patches to CI which found some failures: http://sssd-ci.duckdns.org/logs/job/49/69/summary.html and also Coverity complains: Error: CHECKED_RETURN (CWE-252): sssd-1.14.1/src/lib/sifp/sss_sifp_dbus.c:51: check_return: Calling "dbus_message_append_args_valist" without checking return value (as is done elsewhere 4 out of 5 times).

This was not caused by my patches since the code originates to 2014.

I fixed coverity warnings and CI.

http://sssd-ci.duckdns.org/logs/job/49/75/summary.html

Hi, see some comments inline..

From 6af064012844f3e61b9fa5b502bebe44e4620ffd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pavel=20B=C5=99ezina?= pbrezina@redhat.com Date: Tue, 28 Jun 2016 11:40:16 +0200 Subject: [PATCH 1/7] rdp: add ability to forward reply to the client request

LGTM, but I didn't test yet.

From 2ad0b97f9e435ac5f57caf37479304c16d0424c9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pavel=20B=C5=99ezina?= pbrezina@redhat.com Date: Tue, 28 Jun 2016 15:29:39 +0200 Subject: [PATCH 2/7] sbus: add sbus_request_reply_error()

OK, but what is the plan on the other calls? I wouldn't like us to end up with two competing implementations of the same (again)..

From 0f508095599b3e04573a21a600b4f42e172cc304 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pavel=20B=C5=99ezina?= pbrezina@redhat.com Date: Wed, 29 Jun 2016 12:35:59 +0200 Subject: [PATCH 3/7] sbus: add utility function to simplify message and reply handling

This patch adds the ability to hook DBusMessage to a talloc context to remove the need of calling dbus_message_unref(). It also provides an automatical way to detect error in a reply so the caller does not need to parse it manually and the whole code around DBusError can be avoided.

One code readability nitpick..

+errno_t sbus_check_reply(DBusMessage *reply) +{

• dbus_bool_t bret;
• DBusError error;
• errno_t ret;
• dbus_error_init(&error);
• switch (dbus_message_get_type(reply)) {
• case DBUS_MESSAGE_TYPE_METHOD_RETURN:

•    ret = EOK;
  

•    goto done;
  

Can you use break in the non-failure cases? I would say this would be more readable..

• case DBUS_MESSAGE_TYPE_ERROR:

•    bret = dbus_set_error_from_message(&error, reply);
  

•    if (bret == false) {
  

•        DEBUG(SSSDBG_CRIT_FAILURE, "Unable to read error from message\n");
  

•        ret = EIO;
  

•        goto done;
  

•    }
  

•    DEBUG(SSSDBG_CRIT_FAILURE, "D-Bus error [%s]: %s\n",
  

•          error.name, (error.message == NULL ? "(null)" : error.message));
  

•    ret = sbus_error_to_errno(&error);
  

•    goto done;
  

• default:

•    DEBUG(SSSDBG_CRIT_FAILURE, "Unexpected D-Bus message type?\n");
  

•    ret = ERR_INTERNAL;
  

•    goto done;
  

• }

+done:

• dbus_error_free(&error);
• return ret;

+}

The rest looks good to me.

From 93b4f2e9077f1959d563ba8e4d3f41a9764cc8c6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pavel=20B=C5=99ezina?= pbrezina@redhat.com Date: Wed, 29 Jun 2016 14:03:38 +0200 Subject: [PATCH 4/7] sssctl: use talloc with sifp

LGTM

From 33f5fd436a0fc3d5d1eac970b66c174bb2da6189 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pavel=20B=C5=99ezina?= pbrezina@redhat.com Date: Wed, 29 Jun 2016 14:58:37 +0200 Subject: [PATCH 5/7] failover: mark subdomain service with sd_ prefix

Did you test the failover with subdomain? I wonder if we rely on the service name somewhere, but I can't remember.

But more importantly, also related to the next patch, you noted we don't actually use the GC service for AD subdomains, if that's true shouldn't we remove it completely?

From e4c6e829a1942290d441cec618d477782bbe4c08 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pavel=20B=C5=99ezina?= pbrezina@redhat.com Date: Mon, 27 Jun 2016 13:56:13 +0200 Subject: [PATCH 6/7] ssctl: print active server and server list

---

src/providers/data_provider/dp_iface.c | 4 +- src/providers/data_provider/dp_iface.h | 8 + src/providers/data_provider/dp_iface.xml | 8 + src/providers/data_provider/dp_iface_failover.c | 285 ++++++++++++++++++++++- src/providers/data_provider/dp_iface_generated.c | 52 +++++ src/providers/data_provider/dp_iface_generated.h | 10 + src/providers/fail_over.c | 42 ++++ src/providers/fail_over.h | 4 + src/responder/ifp/ifp_domains.c | 46 ++++ src/responder/ifp/ifp_domains.h | 8 + src/responder/ifp/ifp_iface.c | 4 +- src/responder/ifp/ifp_iface.xml | 10 + src/responder/ifp/ifp_iface_generated.c | 52 +++++ src/responder/ifp/ifp_iface_generated.h | 10 + src/tools/sssctl/sssctl_domains.c | 181 +++++++++++++- 15 files changed, 708 insertions(+), 16 deletions(-)

diff --git a/src/providers/data_provider/dp_iface.c b/src/providers/data_provider/dp_iface.c index 8ed7274f0dd7b59598e2cf21e0dd59d16666df0b..1821b44b0a55a6f0c0406efcca4c88a546e2de09 100644 --- a/src/providers/data_provider/dp_iface.c +++ b/src/providers/data_provider/dp_iface.c @@ -43,7 +43,9 @@ struct iface_dp_backend iface_dp_backend = {

struct iface_dp_failover iface_dp_failover = { {&iface_dp_failover_meta, 0},

• .ListServices = dp_failover_list_services

• .ListServices = dp_failover_list_services,
• .ActiveServer = dp_failover_active_server,
• .ListServers = dp_failover_list_servers

};

static struct sbus_iface_map dp_map[] = { diff --git a/src/providers/data_provider/dp_iface.h b/src/providers/data_provider/dp_iface.h index 76e623d21c413fd68f8f3c9a91ea32fd707dc54d..5c6f0eb2f5dd68b63bda389e6fdd2446ca9efb21 100644 --- a/src/providers/data_provider/dp_iface.h +++ b/src/providers/data_provider/dp_iface.h @@ -69,4 +69,12 @@ errno_t dp_failover_list_services(struct sbus_request *sbus_req, void *dp_cli, const char *domname);

+errno_t dp_failover_active_server(struct sbus_request *sbus_req,

•                              void *dp_cli,
  

•                              const char *service_name);
  

+errno_t dp_failover_list_servers(struct sbus_request *sbus_req,

•                             void *dp_cli,
  

•                             const char *service_name);
  

#endif /* DP_IFACE_H_ */ diff --git a/src/providers/data_provider/dp_iface.xml b/src/providers/data_provider/dp_iface.xml index eab7fc0f1500bf8890030352421da62c134115b9..992848a048ef9fe813d6ae05bbcabd0913ecb277 100644 --- a/src/providers/data_provider/dp_iface.xml +++ b/src/providers/data_provider/dp_iface.xml @@ -22,6 +22,14 @@ <arg name="domain_name" type="s" direction="in" /> <arg name="services" type="as" direction="out" /> </method>

•    <method name="ActiveServer">
  

•        <arg name="service_name" type="s" direction="in" />
  

•        <arg name="server" type="s" direction="out" />
  

•    </method>
  

•    <method name="ListServers">
  

•        <arg name="service_name" type="s" direction="in" />
  

•        <arg name="servers" type="as" direction="out" />
  

•    </method>
  

  </interface>

  <interface name="org.freedesktop.sssd.dataprovider">

diff --git a/src/providers/data_provider/dp_iface_failover.c b/src/providers/data_provider/dp_iface_failover.c index 038791088eeab7e9c5923996db77d2a107ff067d..dbc023233d644006b29935ad1004392929a5c094 100644 --- a/src/providers/data_provider/dp_iface_failover.c +++ b/src/providers/data_provider/dp_iface_failover.c @@ -28,20 +28,202 @@ #include "providers/backend.h" #include "util/util.h"

+static errno_t +dp_failover_list_services_ldap(struct be_ctx *be_ctx,

•                           const char **services,
  

•                           int *_count)
  

+{

• struct be_svc_data *svc;
• int count;
• count = 0;
• DLIST_FOR_EACH(svc, be_ctx->be_fo->svcs) {

•    services[count] = talloc_strdup(services, svc->name);
  

•    if (services[count] == NULL) {
  

•        DEBUG(SSSDBG_CRIT_FAILURE, "talloc_strdup() failed\n");
  

•        return ENOMEM;
  

•    }
  

•    count++;
  

• }
• *_count = count;
• return EOK;

+}

+static errno_t +dp_failover_list_services_ad(struct be_ctx *be_ctx,

•                         struct sss_domain_info *domain,
  

•                         const char **services,
  

•                         int *_count)
  

+{

• char *fo_svc_name = NULL;
• struct be_svc_data *svc;
• errno_t ret;
• int count;
• fo_svc_name = talloc_asprintf(services, "sd_%s", domain->name);
• if (fo_svc_name == NULL) {

•    ret = ENOMEM;
  

•    goto done;
  

• }
• count = 0;
• DLIST_FOR_EACH(svc, be_ctx->be_fo->svcs) {

•    /* Drop each sd_gc_* since this service is not used with AD at all,
  

•     * we only connect to AD_GC for global catalog. */
  

•    if (strncasecmp(svc->name, "sd_gc_", strlen("sd_gc_")) == 0) {
  

•        continue;
  

I asked this in the previous patch, shouldn't we drop the GC service if it's not used? Also, what does "we only connect to AD_GC for global catalog." mean?

•    }
  

•    /* Drop all subdomain services for different domain. */
  

•    if (strncasecmp(svc->name, "sd_", strlen("sd_")) == 0) {
  

•        if (!IS_SUBDOMAIN(domain)) {
  

•            continue;
  

Why would we see sd_ services for non-subdomin?

•        }
  

•        if (strcasecmp(svc->name, fo_svc_name) != 0) {
  

•            continue;
  

•        }
  

•    }
  

•    if (IS_SUBDOMAIN(domain)) {
  

•        /* Drop AD since we connect to subdomain.com for LDAP. */
  

•        if (strcasecmp(svc->name, "AD") == 0) {
  

•            continue;
  

•        }
  

•    }
  

From e74a27309b8a49c0f524bb9d3d66385dd1d33bc1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pavel=20B=C5=99ezina?= pbrezina@redhat.com Date: Thu, 14 Jul 2016 10:49:37 +0200 Subject: [PATCH 7/7] sifp: fix coverity warning

ACK