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
From 2e8e4abb7e79d44f0ce0560daeb7696d9641a684 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 448710993f551298d3a4cdcc19371b8432773478..27c1313cb1f6f614b0c74992d4a768c3051a86ae 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 @@ -360,6 +360,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 e53f968db040a37fbd6a193f87b3671eeabda89d..a70ed20f1816a7e00385edae8a81dd5dad9e9362 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 @@ -145,11 +145,14 @@ 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); + free_req_data(req); return SLAPI_PLUGIN_EXTENDED_SENT_RESULT; } -- 2.1.0
From 80406c884eddeb2ebf35bd12a7ed1a8ddd4af2fe 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 be736dd9c5af4d0b632f1dbc55033fdf738bad46..0ca67030bf667ecd559443842cda166354ce54ce 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 27c1313cb1f6f614b0c74992d4a768c3051a86ae..ce3884805bd3f8276d29d066c2e54897561d0f99 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 @@ -233,6 +233,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 131d5fe646d149057220abf6e0480059af8bf427 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 ce3884805bd3f8276d29d066c2e54897561d0f99..214d6fb23bf21cddac648c0f6b804858d518dcf0 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 @@ -304,26 +304,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; @@ -347,17 +355,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; @@ -756,6 +764,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 { @@ -781,6 +790,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) { @@ -803,6 +813,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; @@ -821,6 +832,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 { @@ -844,6 +856,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) { @@ -864,6 +877,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; @@ -871,6 +885,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; } @@ -878,6 +893,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; } @@ -906,6 +922,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 { @@ -934,6 +951,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 { @@ -964,6 +982,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) @@ -982,6 +1001,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; @@ -993,6 +1013,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; @@ -1012,6 +1033,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 { @@ -1045,6 +1067,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; @@ -1074,27 +1097,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
_______________________________________________ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel