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 <[email protected]> > 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?= <[email protected]> > 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. _______________________________________________ sssd-devel mailing list [email protected] https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
