[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