[SSSD] IFP: Implement methods to get and list domain objects

Jakub Hrozek jhrozek at redhat.com
Tue May 27 12:07:29 UTC 2014


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 at 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 at 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).



More information about the sssd-devel mailing list