On Mon, May 26, 2014 at 10:33:22PM +0200, Pavel Březina wrote:
On 05/26/2014 05:45 PM, Jakub Hrozek wrote:
>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.
>
Hi,
thank you for the review. I'm sending new patches.
>>+
>>+ /* 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
Me neither, thus todo.
>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.
My first idea was to do it as a part of future failover refactoring
(to make the failover context per domain, not per backend). The idea
was to provide a dbus interface for a failover that would retrieve a
list of current servers, also with their status etc.
>>+ 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;
>>+ }
From 10e4b069bec51dab06be674cec4e881adda7bb4c 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 1/4] IFP: Implement domain getters
---
src/responder/ifp/ifp_domains.c | 307 +++++++++++++++++++++++
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, 859 insertions(+)
diff --git a/src/responder/ifp/ifp_domains.c b/src/responder/ifp/ifp_domains.c
index a06ba471c43ac7111693aa0d22a14cf33d168caa..5b34f96e9e6c4b45a1d775818fe15b328d335f18
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"
@@ -27,6 +29,19 @@
#include "responder/common/responder.h"
#include "responder/ifp/ifp_domains.h"
+#define RETURN_DOM_PROP_AS_STRING(dbus_req, pvt_data, out, property) do { \
+ struct sss_domain_info *dom; \
When you declare a variable inside a macro, it's better to give it some
strange name to minimize shadowing other variables. Something like
_pvt_dom maybe.
+
\
+ *(out) = NULL; \
+ \
+ dom = get_domain_info_from_req((dbus_req), (pvt_data)); \
+ if (dom == NULL) { \
+ return; \
+ } \
+ \
+ *(out) = dom->realm; \
dom->property :-)
+} while (0)
+
The rest looks good to me. I'm still reviewing the latest patch, but I
can't apply the one you sent previously to the thread, can you rebase
(or rather regenerate the code, I think something had change in the last
iteration of the GetAll patches that are luckily pushed now).