On Tue, Mar 24, 2015 at 12:05:47PM +0100, Pavel Březina wrote:
Hi,
I'm sending these incomplete patches for review just to get some feed back.
What is missing:
- support for user's extraAttributes (although get invoker is there)
- ListByNameFilter for both groups and users
The code is laid out well and reads good. I would like more non-error
DEBUG messages so that we can follow the code if something goes wrong
and some functions look like they could be made static. See particular
comments inline.
From 2a83625685b909189f309b47d93c6e1ad8e37d3d Mon Sep 17 00:00:00
2001
From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrezina(a)redhat.com>
Date: Wed, 21 Jan 2015 13:38:06 +0100
Subject: [PATCH 1/8] sbus: provide custom error names
[...]
+#define SBUS_ERROR_INTERNAL
"org.freedesktop.sssd.Error.Internal"
+#define SBUS_ERROR_NOT_FOUND "org.freedesktop.sssd.Error.NotFound"
These look good to me, the errors are in the sssd namespace and as other
D-Bus errors use singular.
From 26c17e97d48d580be82feb56add73f11dbfa540a Mon Sep 17 00:00:00
2001
From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrezina(a)redhat.com>
Date: Thu, 29 Jan 2015 15:29:26 +0100
Subject: [PATCH 2/8] sbus: add sbus_opath_decompose[_exact]
[...]
+errno_t
+sbus_opath_decompose_exact(TALLOC_CTX *mem_ctx,
+ const char *object_path,
+ const char *prefix,
+ size_t expected,
+ char ***_components)
+{
+ char **components;
+ size_t len;
+ errno_t ret;
+
+ ret = sbus_opath_decompose(mem_ctx, object_path, prefix,
+ &components, &len);
+ if (ret != EOK) {
+ return ret;
+ }
+
+ if (len != expected) {
+ talloc_free(components);
+ return ERANGE;
I think an internal error code would be appropriate here. If you think
the error would be too specific, what about something like
ERR_SBUS_INVALID_PATH that could be used for similar purposes?
+ }
+
+ if (_components != NULL) {
+ *_components = components;
+ }
+
+ return EOK;
+}
+
const char *
sbus_opath_strip_prefix(const char *object_path,
const char *prefix)
diff --git a/src/tests/cmocka/test_sbus_opath.c b/src/tests/cmocka/test_sbus_opath.c
index 4c57c56e7ebbcac3665c466e1f3d947f7bec92c4..a2b307b014013ac10d16a3ea48b061a0ad21d998
100644
--- a/src/tests/cmocka/test_sbus_opath.c
+++ b/src/tests/cmocka/test_sbus_opath.c
Thanks for the tests.
From 409238d8efc4cb6244a35854a221895ddee06926 Mon Sep 17 00:00:00
2001
From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrezina(a)redhat.com>
Date: Wed, 18 Mar 2015 13:01:07 +0100
Subject: [PATCH 3/8] sbus: add a{sas} get invoker
Looks good, but can you decorate sbus_invoke_get_aDOsasDE() with DEBUG
messages for branches that migth fail?
From a963811b98f3171f7f48cc4a0f48a952634a3436 Mon Sep 17 00:00:00
2001
From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrezina(a)redhat.com>
Date: Thu, 15 Jan 2015 13:52:31 +0100
Subject: [PATCH 4/8] IFP: add org.freedesktop.sssd.infopipe.Users
The interface is sane and conforms to the design page.
diff --git a/src/responder/ifp/ifp_users.c
b/src/responder/ifp/ifp_users.c
new file mode 100644
index 0000000000000000000000000000000000000000..9aec9291c8b4de55f8dfdba9f3b5f5ab36e64a45
--- /dev/null
+++ b/src/responder/ifp/ifp_users.c
@@ -0,0 +1,194 @@
+/*
+ Authors:
+ Pavel Březina <pbrezina(a)redhat.com>
+
+ Copyright (C) 2015 Red Hat
+
+ This program is free software; you can redistribute it and/or modify
+ it under the terms of the GNU General Public License as published by
+ the Free Software Foundation; either version 3 of the License, or
+ (at your option) any later version.
+
+ This program is distributed in the hope that it will be useful,
+ but WITHOUT ANY WARRANTY; without even the implied warranty of
+ MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ GNU General Public License for more details.
+
+ You should have received a copy of the GNU General Public License
+ along with this program. If not, see <
http://www.gnu.org/licenses/>.
+*/
+
+#include <talloc.h>
+#include <tevent.h>
+#include <string.h>
+
+#include "db/sysdb.h"
+#include "util/util.h"
+#include "sbus/sssd_dbus_errors.h"
+#include "responder/common/responder.h"
+#include "responder/common/responder_cache_req.h"
+#include "responder/ifp/ifp_users.h"
+
+char * ifp_users_build_path_from_msg(TALLOC_CTX *mem_ctx,
+ struct sss_domain_info *domain,
+ struct ldb_message *msg)
I wonder why this function isn't static and why it's in the header?
+{
+ const char *uid;
+
+ uid = ldb_msg_find_attr_as_string(msg, SYSDB_UIDNUM, NULL);
+
+ if (uid == NULL) {
+ return NULL;
+ }
+
+ return sbus_opath_compose(mem_ctx, IFP_PATH_USERS, domain->name, uid);
+}
+
+static void ifp_users_find_by_name_done(struct tevent_req *req);
+
+int ifp_users_find_by_name(struct sbus_request *sbus_req,
+ void *data,
+ const char *name)
+{
+ struct ifp_ctx *ctx;
+ struct tevent_req *req;
+
+ ctx = talloc_get_type(data, struct ifp_ctx);
+ if (ctx == NULL) {
+ DEBUG(SSSDBG_CRIT_FAILURE, "Invalid pointer!\n");
+ return ERR_INTERNAL;
+ }
+
+ req = cache_req_user_by_name_send(sbus_req, ctx->rctx->ev, ctx->rctx,
+ ctx->ncache, ctx->neg_timeout, 0,
+ NULL, name);
Missing NULL check.
+
+ tevent_req_set_callback(req, ifp_users_find_by_name_done, sbus_req);
+
+ return EOK;
+}
+
+static void
+ifp_users_find_by_name_done(struct tevent_req *req)
+{
+ DBusError *error;
+ struct sbus_request *sbus_req;
+ struct sss_domain_info *domain;
+ struct ldb_result *result;
+ char *object_path;
+ errno_t ret;
+
+ sbus_req = tevent_req_callback_data(req, struct sbus_request);
+
+ ret = cache_req_user_by_name_recv(sbus_req, req, &result, &domain, NULL);
+ talloc_zfree(req);
+ if (ret == ENOENT) {
+ error = sbus_error_new(sbus_req, SBUS_ERROR_NOT_FOUND,
+ "User not found");
+ goto done;
+ } else if (ret != EOK) {
+ error = sbus_error_new(sbus_req, DBUS_ERROR_FAILED, "Failed to fetch
"
+ "user [%d]: %s\n", ret, sss_strerror(ret));
+ goto done;
+ }
+
+ object_path = ifp_users_build_path_from_msg(sbus_req, domain,
+ result->msgs[0]);
+ if (object_path == NULL) {
+ error = sbus_error_new(sbus_req, SBUS_ERROR_INTERNAL,
+ "Failed to compose object path");
+ goto done;
+ }
+
I would prefer an explicit ret=EOK; here.
+done:
+ if (ret != EOK) {
+ sbus_request_fail_and_finish(sbus_req, error);
+ return;
+ }
+
+ iface_ifp_users_FindByName_finish(sbus_req, object_path);
+ return;
+}
+
+static void ifp_users_find_by_id_done(struct tevent_req *req);
+
+int ifp_users_find_by_id(struct sbus_request *sbus_req,
+ void *data,
+ uint32_t id)
+{
+ struct ifp_ctx *ctx;
+ struct tevent_req *req;
+
+ ctx = talloc_get_type(data, struct ifp_ctx);
+ if (ctx == NULL) {
+ DEBUG(SSSDBG_CRIT_FAILURE, "Invalid pointer!\n");
+ return ERR_INTERNAL;
+ }
+
+ req = cache_req_user_by_id_send(sbus_req, ctx->rctx->ev, ctx->rctx,
+ ctx->ncache, ctx->neg_timeout, 0,
+ NULL, id);
Missing NULL check.
+
+ tevent_req_set_callback(req, ifp_users_find_by_id_done, sbus_req);
+
+ return EOK;
+}
+
+static void
+ifp_users_find_by_id_done(struct tevent_req *req)
+{
+ DBusError *error;
+ struct sbus_request *sbus_req;
+ struct sss_domain_info *domain;
+ struct ldb_result *result;
+ char *object_path;
+ errno_t ret;
+
+ sbus_req = tevent_req_callback_data(req, struct sbus_request);
+
+ ret = cache_req_user_by_id_recv(sbus_req, req, &result, &domain);
+ talloc_zfree(req);
+ if (ret == ENOENT) {
+ error = sbus_error_new(sbus_req, SBUS_ERROR_NOT_FOUND,
+ "User not found");
+ goto done;
+ } else if (ret != EOK) {
+ error = sbus_error_new(sbus_req, DBUS_ERROR_FAILED, "Failed to fetch
"
+ "user [%d]: %s\n", ret, sss_strerror(ret));
+ goto done;
+ }
+
+ object_path = ifp_users_build_path_from_msg(sbus_req, domain,
+ result->msgs[0]);
+ if (object_path == NULL) {
+ error = sbus_error_new(sbus_req, SBUS_ERROR_INTERNAL,
+ "Failed to compose object path");
+ goto done;
+ }
+
I would prefer an explicit ret=EOK; here.
+done:
+ if (ret != EOK) {
+ sbus_request_fail_and_finish(sbus_req, error);
+ return;
+ }
+
+ iface_ifp_users_FindByID_finish(sbus_req, object_path);
+ return;
+}
From 8cbe3a3c867e38ad6fd65f9c08822a5ec3e41680 Mon Sep 17 00:00:00
2001
From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrezina(a)redhat.com>
Date: Mon, 9 Feb 2015 12:02:33 +0100
Subject: [PATCH 5/8] IFP: add org.freedesktop.sssd.infopipe.Users.User
Resolves:
https://fedorahosted.org/sssd/ticket/2150
[...]
index
9aec9291c8b4de55f8dfdba9f3b5f5ab36e64a45..39213035fc29f0a47015aabe17ac634cec793a24 100644
--- a/src/responder/ifp/ifp_users.c
+++ b/src/responder/ifp/ifp_users.c
@@ -24,10 +24,12 @@
#include "db/sysdb.h"
#include "util/util.h"
+#include "util/strtonum.h"
#include "sbus/sssd_dbus_errors.h"
#include "responder/common/responder.h"
#include "responder/common/responder_cache_req.h"
#include "responder/ifp/ifp_users.h"
+#include "responder/ifp/ifp_groups.h"
char * ifp_users_build_path_from_msg(TALLOC_CTX *mem_ctx,
struct sss_domain_info *domain,
@@ -44,6 +46,41 @@ char * ifp_users_build_path_from_msg(TALLOC_CTX *mem_ctx,
return sbus_opath_compose(mem_ctx, IFP_PATH_USERS, domain->name, uid);
}
+errno_t ifp_users_decompose_path(struct sss_domain_info *domains,
+ const char *path,
+ struct sss_domain_info **_domain,
+ uid_t *_uid)
Looks like this one could be static as well.
+{
+ char **parts = NULL;
+ struct sss_domain_info *domain;
+ uid_t uid;
+ errno_t ret;
+
+ ret = sbus_opath_decompose_exact(NULL, path, IFP_PATH_USERS, 2, &parts);
+ if (ret != EOK) {
+ return ret;
+ }
+
+ domain = find_domain_by_name(domains, parts[0], false);
+ if (domain == NULL) {
+ ret = ERR_DOMAIN_NOT_FOUND;
+ goto done;
+ }
+
+ uid = strtouint32(parts[1], NULL, 10);
+ if (errno != 0) {
Since you use errno for error checking, you should set it to zero before
calling strtouint32 (same elsewhere in this patchset).
+ ret = errno;
+ goto done;
+ }
+
+ *_domain = domain;
+ *_uid = uid;
+
+done:
+ talloc_free(parts);
+ return ret;
+}
+
The rest looks good.
From e2d550a4b6b30bdcf264a6285d714029f8309007 Mon Sep 17 00:00:00
2001
From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrezina(a)redhat.com>
Date: Mon, 9 Feb 2015 12:17:37 +0100
Subject: [PATCH 6/8] IFP: add org.freedesktop.sssd.infopipe.Groups
Resolves:
https://fedorahosted.org/sssd/ticket/2150
[...]
+int ifp_groups_find_by_name(struct sbus_request *sbus_req,
+ void *data,
+ const char *name)
+{
+ struct ifp_ctx *ctx;
+ struct tevent_req *req;
+
+ ctx = talloc_get_type(data, struct ifp_ctx);
+ if (ctx == NULL) {
+ DEBUG(SSSDBG_CRIT_FAILURE, "Invalid pointer!\n");
+ return ERR_INTERNAL;
+ }
+
+ req = cache_req_group_by_name_send(sbus_req, ctx->rctx->ev, ctx->rctx,
+ ctx->ncache, ctx->neg_timeout, 0,
+ NULL, name);
+
Missing NULL check.
+ tevent_req_set_callback(req, ifp_groups_find_by_name_done,
sbus_req);
+
+ return EOK;
+}
[...]
+int ifp_groups_find_by_id(struct sbus_request *sbus_req,
+ void *data,
+ uint32_t id)
+{
+ struct ifp_ctx *ctx;
+ struct tevent_req *req;
+
+ ctx = talloc_get_type(data, struct ifp_ctx);
+ if (ctx == NULL) {
+ DEBUG(SSSDBG_CRIT_FAILURE, "Invalid pointer!\n");
+ return ERR_INTERNAL;
+ }
+
+ req = cache_req_group_by_id_send(sbus_req, ctx->rctx->ev, ctx->rctx,
+ ctx->ncache, ctx->neg_timeout, 0,
+ NULL, id);
Missing NULL check.
The rest reads good to me.
From a9d0c396598b767b32dada744603225e517bc845 Mon Sep 17 00:00:00
2001
From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrezina(a)redhat.com>
Date: Tue, 10 Feb 2015 12:19:19 +0100
Subject: [PATCH 7/8] IFP: add org.freedesktop.sssd.infopipe.Groups.Group
Resolves:
https://fedorahosted.org/sssd/ticket/2150
---
src/responder/ifp/ifp_groups.c | 143 +++++++++++++++++++++
src/responder/ifp/ifp_groups.h | 25 ++++
src/responder/ifp/ifp_iface.c | 9 ++
src/responder/ifp/ifp_iface.xml | 9 ++
src/responder/ifp/ifp_iface_generated.c | 50 +++++++
src/responder/ifp/ifp_iface_generated.h | 19 +++
.../ifp/org.freedesktop.sssd.infopipe.conf | 1 +
7 files changed, 256 insertions(+)
diff --git a/src/responder/ifp/ifp_groups.c b/src/responder/ifp/ifp_groups.c
index 9a0e2e2244cb3dd8b203cc2858c3be7c6449e988..f1a93a03675e7583c063ae0adfd434e7d27c708c
100644
--- a/src/responder/ifp/ifp_groups.c
+++ b/src/responder/ifp/ifp_groups.c
@@ -23,6 +23,7 @@
#include "util/util.h"
#include "db/sysdb.h"
+#include "util/strtonum.h"
#include "sbus/sssd_dbus_errors.h"
#include "responder/common/responder.h"
#include "responder/common/responder_cache_req.h"
@@ -43,6 +44,41 @@ char * ifp_groups_build_path_from_msg(TALLOC_CTX *mem_ctx,
return sbus_opath_compose(mem_ctx, IFP_PATH_GROUPS, domain->name, gid);
}
+errno_t ifp_groups_decompose_path(struct sss_domain_info *domains,
+ const char *path,
+ struct sss_domain_info **_domain,
+ gid_t *_gid)
+{
+ char **parts = NULL;
+ struct sss_domain_info *domain;
+ gid_t gid;
+ errno_t ret;
+
+ ret = sbus_opath_decompose_exact(NULL, path, IFP_PATH_GROUPS, 2, &parts);
+ if (ret != EOK) {
+ return ret;
+ }
+
+ domain = find_domain_by_name(domains, parts[0], false);
+ if (domain == NULL) {
+ ret = ERR_DOMAIN_NOT_FOUND;
+ goto done;
+ }
+
Please zero out errno here.
Looking at the interface, I wonder about the members. Are they
transitive or just direct members?