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?= <[email protected]> > 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?= <[email protected]> > 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?= <[email protected]> > 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?= <[email protected]> > 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?= <[email protected]> > 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?= <[email protected]> > 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?= <[email protected]> > Date: Thu, 14 Jul 2016 10:49:37 +0200 > Subject: [PATCH 7/7] sifp: fix coverity warning ACK _______________________________________________ sssd-devel mailing list [email protected] https://lists.fedorahosted.org/admin/lists/[email protected]
