On Thu, May 22, 2014 at 07:57:28PM +0200, Pavel Březina wrote:
On 05/21/2014 11:03 PM, Jakub Hrozek wrote:
>Hi,
>
>the attaches two patches implement DBus methods to retrieve SSSD domains
>using the system bus as well as two example getters that leverage the
>infrastructure to Get and GetAll the object properties.
>
>Pavel, I know you have a large patchset in a private tree that
>implements the other getters and moves the domain methods into a
>separate file. I'm fine with merging your and mine patches, I just
>wanted to get the ball rolling on the list. Feel free to reply with your
>patchset.
Hi,
see the attachment.
I squash all my patches to these five - mainly discarded the moving
the code around.
I kept you as the author for the methods as I made only very minor
changes there. However, I took the ownership for the properties
since there only very little of your original code remains there - I
hope you don't mind.
The next three patches implements the SSSD components interface
required for the OpenLMI provider. I'm sending it to this thread
since it depends on the domain patches anyway - there is enough
dependency fuzz already.
From 7e55b23741ed749e95e9d33184b0fd702b86bb46 Mon Sep 17 00:00:00
2001
From: Jakub Hrozek <jhrozek(a)redhat.com>
Date: Tue, 22 Apr 2014 15:38:08 +0200
Subject: [PATCH 1/5] IFP: Add ListDomains and FindDomainByName
This one was acked by Pavel separately.
From d3d93a3d9ae3d2818c6fd3b78662aab27a3ace36 Mon Sep 17 00:00:00
2001
From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrezina(a)redhat.com>
Date: Thu, 22 May 2014 19:01:35 +0200
Subject: [PATCH 2/5] IFP: Implement domain getters
See inline for some nitpicks. In general, the code seems to work well.
As par of the review exercise, I started writing some unit tests, but I
don't want to block your code from master any longer.
---
src/responder/ifp/ifp_domains.c | 348 ++++++++++++++++++++++++++
src/responder/ifp/ifp_domains.h | 60 +++++
src/responder/ifp/ifp_iface.xml | 19 ++
src/responder/ifp/ifp_iface_generated.c | 415 ++++++++++++++++++++++++++++++++
src/responder/ifp/ifp_iface_generated.h | 39 +++
src/responder/ifp/ifpsrv.c | 19 ++
6 files changed, 900 insertions(+)
diff --git a/src/responder/ifp/ifp_domains.c b/src/responder/ifp/ifp_domains.c
index a06ba471c43ac7111693aa0d22a14cf33d168caa..ad0c4132711d21a009c2abc9e21a417144936bcd
100644
--- a/src/responder/ifp/ifp_domains.c
+++ b/src/responder/ifp/ifp_domains.c
@@ -20,6 +20,8 @@
*/
#include <talloc.h>
+#include <tevent.h>
+#include <string.h>
#include "db/sysdb.h"
#include "util/util.h"
@@ -232,3 +234,349 @@ static void ifp_find_domain_by_name_process(struct tevent_req
*req)
DEBUG(SSSDBG_OP_FAILURE, "Could not finish request!\n");
}
}
+
+static struct sss_domain_info *
+get_domain_info_from_req(struct sbus_request *req, void *data)
I would prefer a different name than 'req' for sbus_request. 'req' is
used through the code as a tevent_req instance.
+{
+ struct ifp_ctx *ctx = NULL;
+ struct sss_domain_info *domains = NULL;
+ struct sss_domain_info *iter = NULL;
+ const char *path = dbus_message_get_path(req->message);
+ const char *safe_name = NULL;
+ char *name = NULL;
+
+ ctx = talloc_get_type(data, struct ifp_ctx);
+ if (ctx == NULL) {
+ DEBUG(SSSDBG_CRIT_FAILURE, "Invalid pointer!\n");
+ return NULL;
+ }
+
+ safe_name = ifp_path_strip_prefix(path, INFOPIPE_DOMAIN_PATH_PFX "/");
I think raw_name would be a better name. I would use safe_name for
something that was unsafe and was escaped to be safe.
+ if (safe_name == NULL) {
+ return NULL;
+ }
+
+ name = ifp_bus_path_unescape(NULL, safe_name);
Can you use sbus_req as the parent context here? I know you free the
memory in this function, but allocating on the request would be
future-proof when we refactor this code..
+ if (name == NULL) {
+ return NULL;
+ }
+
+ DEBUG(SSSDBG_TRACE_INTERNAL, "Looking for domain %s\n", name);
+
+ domains = ctx->rctx->domains;
+ for (iter = domains; iter != NULL; iter = get_next_domain(iter, true)) {
+ if (strcasecmp(iter->name, name) == 0) {
+ break;
+ }
+ }
+
+ talloc_free(name);
+ return iter;
+}
+
+static void get_server_list(struct sbus_request *dbus_req,
+ void *data,
+ const char ***_out,
+ int *_out_len,
+ bool backup)
+{
+ static const char *srv[] = {"_srv_"};
+ struct sss_domain_info *dom = NULL;
+ struct ifp_ctx *ctx = NULL;
+ const char *conf_path = NULL;
+ const char *option = NULL;
+ const char **out = NULL;
+ char **servers = NULL;
+ int num_servers;
+ errno_t ret;
+ int i;
+
+ *_out = NULL;
+ *_out_len = 0;
+
+ dom = get_domain_info_from_req(dbus_req, data);
+ if (dom == NULL) {
+ return;
+ }
+
+ if (dom->parent != NULL) {
+ /* subdomains are not present in configuration */
+ ret = ENOENT;
+ goto done;
+ }
+
+ ctx = talloc_get_type(data, struct ifp_ctx);
+ if (ctx == NULL) {
+ DEBUG(SSSDBG_CRIT_FAILURE, "Invalid ifp context!\n");
+ ret = ENOMEM;
+ goto done;
+ }
+
+ conf_path = talloc_asprintf(dbus_req, CONFDB_DOMAIN_PATH_TMPL, dom->name);
+ if (conf_path == NULL) {
+ ret = ENOMEM;
+ goto done;
+ }
+
+ /* TODO: replace hardcoded values with option names from the provider */
I don't like this at all, but I can't see another way with the current
code. Maybe we should add a new ID target for getting info about
domains? I think the performance would be a non-issue since all that
would be needed is an sbus round trip to the DP that would just reply
right away. But any DP changes should be discussed on the list first --
for now, just please file a ticket.
+ if (strcasecmp(dom->provider, "ldap") == 0) {
+ option = backup == false ? "ldap_uri" : "ldap_backup_uri";
+ } else if (strcasecmp(dom->provider, "ipa") == 0) {
+ option = backup == false ? "ipa_server" :
"ipa_backup_server";
+ } else if (strcasecmp(dom->provider, "ad") == 0) {
+ option = backup == false ? "ad_server" :
"ad_backup_server";
+ } else {
+ ret = EINVAL;
+ goto done;
+ }
+
+ ret = confdb_get_string_as_list(ctx->rctx->cdb, dbus_req, conf_path,
+ option, &servers);
+ if (ret != EOK) {
+ goto done;
+ }
+
+ for (num_servers = 0; servers[num_servers] != NULL; num_servers++);
+
+ if (num_servers == 0) {
+ ret = ENOENT;
+ goto done;
+ }
+
+ out = talloc_zero_array(dbus_req, const char*, num_servers);
+ if (out == NULL) {
+ ret = ENOMEM;
+ goto done;
+ }
+
+ for (i = 0; i < num_servers; i++) {
+ out[i] = talloc_steal(out, servers[i]);
+ }
+
+ *_out = out;
+ *_out_len = num_servers;
+
+ ret = EOK;
+
+done:
+ if (ret == ENOENT) {
+ *_out = srv;
+ *_out_len = 1;
+ }
+
+ return;
+}
+
+void ifp_dom_get_name(struct sbus_request *dbus_req,
+ void *data,
+ const char **_out)
+{
+ struct sss_domain_info *dom;
+
+ *_out = NULL;
+
+ dom = get_domain_info_from_req(dbus_req, data);
+ if (dom == NULL) {
+ return;
+ }
+
+ *_out = dom->name;
+}
A general comment about the string getters -- could we have a macro that would
wrap the repeated pattern? Maybe something like (pseudocode):
#define dom_get_str(dbus_req, data, _out, prop) \
struct sss_domain_info *dom; \
dom = get_domain_info_from_req(dbus_req, data); \
*_out = dom ? dom->prop : NULL;
Then you could reduce the getter to a single line:
dom_get_str(dbus_req, data, _out, name);
[snip, the other getters generally look good to me]
+void ifp_dom_get_min_id(struct sbus_request *dbus_req,
+ void *data,
+ uint32_t *_out)
+{
+ struct sss_domain_info *dom;
+
+ *_out = 0;
The min_id default should be 1, I think. That's the default for min_id
in confdb anyway. And our responders explicitly filter any request by ID
for ID 0.
+
+ dom = get_domain_info_from_req(dbus_req, data);
+ if (dom == NULL) {
+ return;
+ }
+
+ *_out = dom->id_min;
+}
+
+void ifp_dom_get_max_id(struct sbus_request *dbus_req,
+ void *data,
+ uint32_t *_out)
+{
+ struct sss_domain_info *dom;
+
+ *_out = 0;
Here the default is OK, because 0 means 'unlimited'.
+
+ dom = get_domain_info_from_req(dbus_req, data);
+ if (dom == NULL) {
+ return;
+ }
+
+ *_out = dom->id_max;
+}
+
+void ifp_dom_get_realm(struct sbus_request *dbus_req,
+ void *data,
+ const char **_out)
+{
+ struct sss_domain_info *dom;
+
+ *_out = NULL;
+
+ dom = get_domain_info_from_req(dbus_req, data);
+ if (dom == NULL) {
+ return;
+ }
+
+ *_out = dom->realm;
+}
+
+void ifp_dom_get_forest(struct sbus_request *dbus_req,
+ void *data,
+ const char **_out)
+{
+ struct sss_domain_info *dom;
+
+ *_out = NULL;
+
+ dom = get_domain_info_from_req(dbus_req, data);
+ if (dom == NULL) {
+ return;
+ }
+
+ *_out = dom->forest;
+}
+
+void ifp_dom_get_login_format(struct sbus_request *dbus_req,
+ void *data,
+ const char **_out)
+{
+ struct sss_domain_info *dom;
+
+ *_out = NULL;
+
+ dom = get_domain_info_from_req(dbus_req, data);
+ if (dom == NULL) {
+ return;
+ }
+
+ *_out = dom->names->re_pattern;
+}
+
+void ifp_dom_get_fqdn_format(struct sbus_request *dbus_req,
+ void *data,
+ const char **_out)
+{
+ struct sss_domain_info *dom;
+
+ *_out = NULL;
+
+ dom = get_domain_info_from_req(dbus_req, data);
+ if (dom == NULL) {
+ return;
+ }
+
+ *_out = dom->names->fq_fmt;
+}
+
+void ifp_dom_get_enumerable(struct sbus_request *dbus_req,
+ void *data,
+ bool *_out)
+{
+ struct sss_domain_info *dom;
+
+ *_out = false;
+
+ dom = get_domain_info_from_req(dbus_req, data);
+ if (dom == NULL) {
+ return;
+ }
+
+ *_out = dom->enumerate;
+}
+
+void ifp_dom_get_use_fqdn(struct sbus_request *dbus_req,
+ void *data,
+ bool *_out)
+{
+ struct sss_domain_info *dom;
+
+ *_out = false;
+
+ dom = get_domain_info_from_req(dbus_req, data);
+ if (dom == NULL) {
+ return;
+ }
+
+ *_out = dom->fqnames;
+}
+
+void ifp_dom_get_subdomain(struct sbus_request *dbus_req,
+ void *data,
+ bool *_out)
+{
+ struct sss_domain_info *dom;
+
+ *_out = NULL;
Why are you setting a bool default to NULL? I guess you wanted to use
false here?
+
+ dom = get_domain_info_from_req(dbus_req, data);
+ if (dom == NULL) {
+ return;
+ }
+
+ *_out = dom->parent ? true : false;
+}
+
+void ifp_dom_get_parent_domain(struct sbus_request *dbus_req,
+ void *data,
+ const char **_out)
+{
+ struct sss_domain_info *dom;
+
+ *_out = NULL;
+
+ dom = get_domain_info_from_req(dbus_req, data);
+ if (dom == NULL) {
+ return;
+ }
+
+ if (dom->parent == NULL) {
+ *_out = "/";
+ return;
+ }
+
+ *_out = ifp_reply_objpath(dbus_req, INFOPIPE_DOMAIN_PATH_PFX,
+ dom->parent->name);
+}
The rest of the patch looks good to me.
I'll send review of other patches separately so you can work on
addressing the issues in parallel.