On Fri, Mar 13, 2015 at 03:17:10PM +0100, Jakub Hrozek wrote:
> On Fri, Mar 13, 2015 at 11:55:09AM +0100, Sumit Bose wrote:
> > On Wed, Mar 04, 2015 at 06:35:22PM +0100, Sumit Bose wrote:
> > > Hi,
> > >
> > > this patch series improves error reporting of the extdom plugin
> > > especially on the client side. Currently there is only SSSD ticket
> > > https://fedorahosted.org/sssd/ticket/2463 . Shall I create a
> > > corresponding FreeIPA ticket as well?
> > >
> > > In the third patch I already added a handful of new error messages.
> > > Suggestions for more messages are welcome.
> > >
> > > bye,
> > > Sumit
> >
> > Rebased versions attached.
> >
> > bye,
> > Sumit
>
> The patches look good and work fine. I admit I cheated a bit and
> modified the code to return a failure. Then I saw on the client:
>
> [sssd[be[ipa.example.com]]] [ipa_s2n_exop_send] (0x0400): Executing extended
> operation
> [sssd[be[ipa.example.com]]] [ipa_s2n_exop_done] (0x0040):
> ldap_extended_operation result: Operations
> error(1), Failed to create fully qualified name.
> [sssd[be[ipa.example.com]]] [ipa_s2n_exop_done] (0x0040):
> ldap_extended_operation failed, server logs might contain more details.
> [sssd[be[ipa.example.com]]] [ipa_s2n_get_user_done] (0x0040): s2n exop
> request failed.
>
> I just saw one typo:
>
> > @@ -918,6 +934,7 @@ static int handle_sid_request(struct ipa_extdom_ctx
> > *ctx,
> > ret = sss_nss_getorigbyname(pwd.pw_name, &kv_list, &id_type);
> > if (ret != 0 || !(id_type == SSS_ID_TYPE_UID
> > || id_type == SSS_ID_TYPE_BOTH)) {
> > + set_err_msg(req, "Failed ot read original data");
> ~~~~~~~~~
> > if (ret == ENOENT) {
> > ret = LDAP_NO_SUCH_OBJECT;
> > } else {
>
> And a compilation warning caused by previous patches.
>
> So ACK provided the typo is fixed prior to pushing the patch.
>
Please find attached a new version where the typo is fixed.
bye,
Sumit
From 9638b0a61144a481a0a2c8757d4e3a10e1f44b12 Mon Sep 17 00:00:00 2001
From: Sumit Bose <sb...@redhat.com>
Date: Mon, 2 Feb 2015 00:52:10 +0100
Subject: [PATCH 137/139] extdom: add err_msg member to request context
---
daemons/ipa-slapi-plugins/ipa-extdom-extop/ipa_extdom.h | 1 +
daemons/ipa-slapi-plugins/ipa-extdom-extop/ipa_extdom_common.c | 1 +
daemons/ipa-slapi-plugins/ipa-extdom-extop/ipa_extdom_extop.c | 5 ++++-
3 files changed, 6 insertions(+), 1 deletion(-)
diff --git a/daemons/ipa-slapi-plugins/ipa-extdom-extop/ipa_extdom.h
b/daemons/ipa-slapi-plugins/ipa-extdom-extop/ipa_extdom.h
index
d4c851169ddadc869a59c53075f9fc7f33321085..421f6c6ea625aba2db7e9ffc84115b3647673699
100644
--- a/daemons/ipa-slapi-plugins/ipa-extdom-extop/ipa_extdom.h
+++ b/daemons/ipa-slapi-plugins/ipa-extdom-extop/ipa_extdom.h
@@ -116,6 +116,7 @@ struct extdom_req {
gid_t gid;
} posix_gid;
} data;
+ char *err_msg;
};
struct extdom_res {
diff --git a/daemons/ipa-slapi-plugins/ipa-extdom-extop/ipa_extdom_common.c
b/daemons/ipa-slapi-plugins/ipa-extdom-extop/ipa_extdom_common.c
index
47bcb179f04e08c64d92f55809b84f2d59622344..c2fd42f13fca97587ddc4c12b560e590462f121b
100644
--- a/daemons/ipa-slapi-plugins/ipa-extdom-extop/ipa_extdom_common.c
+++ b/daemons/ipa-slapi-plugins/ipa-extdom-extop/ipa_extdom_common.c
@@ -356,6 +356,7 @@ void free_req_data(struct extdom_req *req)
break;
}
+ free(req->err_msg);
free(req);
}
diff --git a/daemons/ipa-slapi-plugins/ipa-extdom-extop/ipa_extdom_extop.c
b/daemons/ipa-slapi-plugins/ipa-extdom-extop/ipa_extdom_extop.c
index
dc7785877fc321ddaa5b6967d1c1b06cb454bbbf..708d0e4a2fc9da4f87a24a49c945587049f7280f
100644
--- a/daemons/ipa-slapi-plugins/ipa-extdom-extop/ipa_extdom_extop.c
+++ b/daemons/ipa-slapi-plugins/ipa-extdom-extop/ipa_extdom_extop.c
@@ -149,12 +149,15 @@ static int ipa_extdom_extop(Slapi_PBlock *pb)
rc = LDAP_SUCCESS;
done:
- free_req_data(req);
+ if (req->err_msg != NULL) {
+ err_msg = req->err_msg;
+ }
if (err_msg != NULL) {
LOG("%s", err_msg);
}
slapi_send_ldap_result(pb, rc, NULL, err_msg, 0, NULL);
ber_bvfree(ret_val);
+ free_req_data(req);
return SLAPI_PLUGIN_EXTENDED_SENT_RESULT;
}
--
2.1.0
From b52bc91cf9e10a779446bf399f138eb3929cfb55 Mon Sep 17 00:00:00 2001
From: Sumit Bose <sb...@redhat.com>
Date: Mon, 2 Feb 2015 00:53:06 +0100
Subject: [PATCH 138/139] extdom: add add_err_msg() with test
---
.../ipa-extdom-extop/ipa_extdom.h | 1 +
.../ipa-extdom-extop/ipa_extdom_cmocka_tests.c | 43 ++++++++++++++++++++++
.../ipa-extdom-extop/ipa_extdom_common.c | 23 ++++++++++++
3 files changed, 67 insertions(+)
diff --git a/daemons/ipa-slapi-plugins/ipa-extdom-extop/ipa_extdom.h
b/daemons/ipa-slapi-plugins/ipa-extdom-extop/ipa_extdom.h
index
421f6c6ea625aba2db7e9ffc84115b3647673699..0d5d55d2fb0ece95466b0225b145a4edeef18efa
100644
--- a/daemons/ipa-slapi-plugins/ipa-extdom-extop/ipa_extdom.h
+++ b/daemons/ipa-slapi-plugins/ipa-extdom-extop/ipa_extdom.h
@@ -185,4 +185,5 @@ int getgrnam_r_wrapper(size_t buf_max, const char *name,
struct group *grp, char **_buf, size_t *_buf_len);
int getgrgid_r_wrapper(size_t buf_max, gid_t gid,
struct group *grp, char **_buf, size_t *_buf_len);
+void set_err_msg(struct extdom_req *req, const char *format, ...);
#endif /* _IPA_EXTDOM_H_ */
diff --git
a/daemons/ipa-slapi-plugins/ipa-extdom-extop/ipa_extdom_cmocka_tests.c
b/daemons/ipa-slapi-plugins/ipa-extdom-extop/ipa_extdom_cmocka_tests.c
index
d5bacd7e8c9dc0a71eea70162406c7e5b67384ad..586b58b0fd4c7610e9cb4643b6dae04f9d22b8ab
100644
--- a/daemons/ipa-slapi-plugins/ipa-extdom-extop/ipa_extdom_cmocka_tests.c
+++ b/daemons/ipa-slapi-plugins/ipa-extdom-extop/ipa_extdom_cmocka_tests.c
@@ -213,6 +213,47 @@ void test_getgrgid_r_wrapper(void **state)
free(buf);
}
+void extdom_req_setup(void **state)
+{
+ struct extdom_req *req;
+
+ req = calloc(sizeof(struct extdom_req), 1);
+ assert_non_null(req);
+
+ *state = req;
+}
+
+void extdom_req_teardown(void **state)
+{
+ struct extdom_req *req;
+
+ req = (struct extdom_req *) *state;
+
+ free_req_data(req);
+}
+
+void test_set_err_msg(void **state)
+{
+ struct extdom_req *req;
+
+ req = (struct extdom_req *) *state;
+ assert_null(req->err_msg);
+
+ set_err_msg(NULL, NULL);
+ assert_null(req->err_msg);
+
+ set_err_msg(req, NULL);
+ assert_null(req->err_msg);
+
+ set_err_msg(req, "Test [%s][%d].", "ABCD", 1234);
+ assert_non_null(req->err_msg);
+ assert_string_equal(req->err_msg, "Test [ABCD][1234].");
+
+ set_err_msg(req, "2nd Test [%s][%d].", "ABCD", 1234);
+ assert_non_null(req->err_msg);
+ assert_string_equal(req->err_msg, "Test [ABCD][1234].");
+}
+
int main(int argc, const char *argv[])
{
const UnitTest tests[] = {
@@ -220,6 +261,8 @@ int main(int argc, const char *argv[])
unit_test(test_getpwuid_r_wrapper),
unit_test(test_getgrnam_r_wrapper),
unit_test(test_getgrgid_r_wrapper),
+ unit_test_setup_teardown(test_set_err_msg,
+ extdom_req_setup, extdom_req_teardown),
};
return run_tests(tests);
diff --git a/daemons/ipa-slapi-plugins/ipa-extdom-extop/ipa_extdom_common.c
b/daemons/ipa-slapi-plugins/ipa-extdom-extop/ipa_extdom_common.c
index
c2fd42f13fca97587ddc4c12b560e590462f121b..e05c005da4efcdbbee386f9e73ef3ef889e1a3c2
100644
--- a/daemons/ipa-slapi-plugins/ipa-extdom-extop/ipa_extdom_common.c
+++ b/daemons/ipa-slapi-plugins/ipa-extdom-extop/ipa_extdom_common.c
@@ -229,6 +229,29 @@ done:
return ret;
}
+void set_err_msg(struct extdom_req *req, const char *format, ...)
+{
+ int ret;
+ va_list ap;
+
+ if (req == NULL) {
+ return;
+ }
+
+ if (format == NULL || req->err_msg != NULL) {
+ /* Do not override an existing error message. */
+ return;
+ }
+ va_start(ap, format);
+
+ ret = vasprintf(&req->err_msg, format, ap);
+ if (ret == -1) {
+ req->err_msg = strdup("vasprintf failed.\n");
+ }
+
+ va_end(ap);
+}
+
int parse_request_data(struct berval *req_val, struct extdom_req **_req)
{
BerElement *ber = NULL;
--
2.1.0
From 6e6cbc67f084ed745685f04f5b7a50bf65147c95 Mon Sep 17 00:00:00 2001
From: Sumit Bose <sb...@redhat.com>
Date: Wed, 4 Mar 2015 13:37:50 +0100
Subject: [PATCH 139/139] extdom: add selected error messages
---
.../ipa-extdom-extop/ipa_extdom_common.c | 51 ++++++++++++++++------
1 file changed, 38 insertions(+), 13 deletions(-)
diff --git a/daemons/ipa-slapi-plugins/ipa-extdom-extop/ipa_extdom_common.c
b/daemons/ipa-slapi-plugins/ipa-extdom-extop/ipa_extdom_common.c
index
e05c005da4efcdbbee386f9e73ef3ef889e1a3c2..4452d456bcabc211a0ca5814d24247c43cf95a91
100644
--- a/daemons/ipa-slapi-plugins/ipa-extdom-extop/ipa_extdom_common.c
+++ b/daemons/ipa-slapi-plugins/ipa-extdom-extop/ipa_extdom_common.c
@@ -300,26 +300,34 @@ int parse_request_data(struct berval *req_val, struct
extdom_req **_req)
* }
*/
+ req = calloc(sizeof(struct extdom_req), 1);
+ if (req == NULL) {
+ /* Since we return req even in the case of an error we make sure is is
+ * always safe to call free_req_data() on the returned data. */
+ *_req = NULL;
+ return LDAP_OPERATIONS_ERROR;
+ }
+
+ *_req = req;
+
if (req_val == NULL || req_val->bv_val == NULL || req_val->bv_len == 0) {
+ set_err_msg(req, "Missing request data");
return LDAP_PROTOCOL_ERROR;
}
ber = ber_init(req_val);
if (ber == NULL) {
+ set_err_msg(req, "Cannot initialize BER struct");
return LDAP_PROTOCOL_ERROR;
}
tag = ber_scanf(ber, "{ee", &input_type, &request_type);
if (tag == LBER_ERROR) {
ber_free(ber, 1);
+ set_err_msg(req, "Cannot read input and request type");
return LDAP_PROTOCOL_ERROR;
}
- req = calloc(sizeof(struct extdom_req), 1);
- if (req == NULL) {
- return LDAP_OPERATIONS_ERROR;
- }
-
req->input_type = input_type;
req->request_type = request_type;
@@ -343,17 +351,15 @@ int parse_request_data(struct berval *req_val, struct
extdom_req **_req)
break;
default:
ber_free(ber, 1);
- free(req);
+ set_err_msg(req, "Unknown input type");
return LDAP_PROTOCOL_ERROR;
}
ber_free(ber, 1);
if (tag == LBER_ERROR) {
- free(req);
+ set_err_msg(req, "Failed to decode BER data");
return LDAP_PROTOCOL_ERROR;
}
- *_req = req;
-
return LDAP_SUCCESS;
}
@@ -715,6 +721,7 @@ static int pack_ber_name(const char *domain_name, const
char *name,
}
static int handle_uid_request(struct ipa_extdom_ctx *ctx,
+ struct extdom_req *req,
enum request_types request_type, uid_t uid,
const char *domain_name, struct berval **berval)
{
@@ -738,6 +745,7 @@ static int handle_uid_request(struct ipa_extdom_ctx *ctx,
if (ret == ENOENT) {
ret = LDAP_NO_SUCH_OBJECT;
} else {
+ set_err_msg(req, "Failed to lookup SID by UID");
ret = LDAP_OPERATIONS_ERROR;
}
goto done;
@@ -760,6 +768,7 @@ static int handle_uid_request(struct ipa_extdom_ctx *ctx,
ret = sss_nss_getorigbyname(pwd.pw_name, &kv_list, &id_type);
if (ret != 0 || !(id_type == SSS_ID_TYPE_UID
|| id_type == SSS_ID_TYPE_BOTH)) {
+ set_err_msg(req, "Failed to read original data");
if (ret == ENOENT) {
ret = LDAP_NO_SUCH_OBJECT;
} else {
@@ -785,6 +794,7 @@ done:
}
static int handle_gid_request(struct ipa_extdom_ctx *ctx,
+ struct extdom_req *req,
enum request_types request_type, gid_t gid,
const char *domain_name, struct berval **berval)
{
@@ -807,6 +817,7 @@ static int handle_gid_request(struct ipa_extdom_ctx *ctx,
if (ret == ENOENT) {
ret = LDAP_NO_SUCH_OBJECT;
} else {
+ set_err_msg(req, "Failed to lookup SID by GID");
ret = LDAP_OPERATIONS_ERROR;
}
goto done;
@@ -829,6 +840,7 @@ static int handle_gid_request(struct ipa_extdom_ctx *ctx,
ret = sss_nss_getorigbyname(grp.gr_name, &kv_list, &id_type);
if (ret != 0 || !(id_type == SSS_ID_TYPE_GID
|| id_type == SSS_ID_TYPE_BOTH)) {
+ set_err_msg(req, "Failed to read original data");
if (ret == ENOENT) {
ret = LDAP_NO_SUCH_OBJECT;
} else {
@@ -852,6 +864,7 @@ done:
}
static int handle_sid_request(struct ipa_extdom_ctx *ctx,
+ struct extdom_req *req,
enum request_types request_type, const char *sid,
struct berval **berval)
{
@@ -872,6 +885,7 @@ static int handle_sid_request(struct ipa_extdom_ctx *ctx,
if (ret == ENOENT) {
ret = LDAP_NO_SUCH_OBJECT;
} else {
+ set_err_msg(req, "Failed to lookup name by SID");
ret = LDAP_OPERATIONS_ERROR;
}
goto done;
@@ -879,6 +893,7 @@ static int handle_sid_request(struct ipa_extdom_ctx *ctx,
sep = strchr(fq_name, SSSD_DOMAIN_SEPARATOR);
if (sep == NULL) {
+ set_err_msg(req, "Failed to split fully qualified name");
ret = LDAP_OPERATIONS_ERROR;
goto done;
}
@@ -886,6 +901,7 @@ static int handle_sid_request(struct ipa_extdom_ctx *ctx,
object_name = strndup(fq_name, (sep - fq_name));
domain_name = strdup(sep + 1);
if (object_name == NULL || domain_name == NULL) {
+ set_err_msg(req, "Missing name or domain");
ret = LDAP_OPERATIONS_ERROR;
goto done;
}
@@ -918,6 +934,7 @@ static int handle_sid_request(struct ipa_extdom_ctx *ctx,
ret = sss_nss_getorigbyname(pwd.pw_name, &kv_list, &id_type);
if (ret != 0 || !(id_type == SSS_ID_TYPE_UID
|| id_type == SSS_ID_TYPE_BOTH)) {
+ set_err_msg(req, "Failed to read original data");
if (ret == ENOENT) {
ret = LDAP_NO_SUCH_OBJECT;
} else {
@@ -950,6 +967,7 @@ static int handle_sid_request(struct ipa_extdom_ctx *ctx,
ret = sss_nss_getorigbyname(grp.gr_name, &kv_list, &id_type);
if (ret != 0 || !(id_type == SSS_ID_TYPE_GID
|| id_type == SSS_ID_TYPE_BOTH)) {
+ set_err_msg(req, "Failed to read original data");
if (ret == ENOENT) {
ret = LDAP_NO_SUCH_OBJECT;
} else {
@@ -980,6 +998,7 @@ done:
}
static int handle_name_request(struct ipa_extdom_ctx *ctx,
+ struct extdom_req *req,
enum request_types request_type,
const char *name, const char *domain_name,
struct berval **berval)
@@ -998,6 +1017,7 @@ static int handle_name_request(struct ipa_extdom_ctx *ctx,
domain_name);
if (ret == -1) {
ret = LDAP_OPERATIONS_ERROR;
+ set_err_msg(req, "Failed to create fully qualified name");
fq_name = NULL; /* content is undefined according to
asprintf(3) */
goto done;
@@ -1009,6 +1029,7 @@ static int handle_name_request(struct ipa_extdom_ctx *ctx,
if (ret == ENOENT) {
ret = LDAP_NO_SUCH_OBJECT;
} else {
+ set_err_msg(req, "Failed to lookup SID by name");
ret = LDAP_OPERATIONS_ERROR;
}
goto done;
@@ -1028,6 +1049,7 @@ static int handle_name_request(struct ipa_extdom_ctx *ctx,
ret = sss_nss_getorigbyname(pwd.pw_name, &kv_list, &id_type);
if (ret != 0 || !(id_type == SSS_ID_TYPE_UID
|| id_type == SSS_ID_TYPE_BOTH)) {
+ set_err_msg(req, "Failed to read original data");
if (ret == ENOENT) {
ret = LDAP_NO_SUCH_OBJECT;
} else {
@@ -1068,6 +1090,7 @@ static int handle_name_request(struct ipa_extdom_ctx *ctx,
if (ret == ENOENT) {
ret = LDAP_NO_SUCH_OBJECT;
} else {
+ set_err_msg(req, "Failed to read original data");
ret = LDAP_OPERATIONS_ERROR;
}
goto done;
@@ -1097,27 +1120,29 @@ int handle_request(struct ipa_extdom_ctx *ctx, struct
extdom_req *req,
switch (req->input_type) {
case INP_POSIX_UID:
- ret = handle_uid_request(ctx, req->request_type,
+ ret = handle_uid_request(ctx, req, req->request_type,
req->data.posix_uid.uid,
req->data.posix_uid.domain_name, berval);
break;
case INP_POSIX_GID:
- ret = handle_gid_request(ctx, req->request_type,
+ ret = handle_gid_request(ctx, req, req->request_type,
req->data.posix_gid.gid,
req->data.posix_uid.domain_name, berval);
break;
case INP_SID:
- ret = handle_sid_request(ctx, req->request_type, req->data.sid,
berval);
+ ret = handle_sid_request(ctx, req, req->request_type, req->data.sid,
+ berval);
break;
case INP_NAME:
- ret = handle_name_request(ctx, req->request_type,
+ ret = handle_name_request(ctx, req, req->request_type,
req->data.name.object_name,
req->data.name.domain_name, berval);
break;
default:
+ set_err_msg(req, "Unknown input type");
ret = LDAP_PROTOCOL_ERROR;
goto done;
}
--
2.1.0
--
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code
