-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 07/30/2009 08:45 PM, Stephen Gallagher wrote:
> I'm going to nack the user notification patch. I think we need to think
> some more about this. The sysdb has an interface in place for returning
> ENOENT when trying to delete, we're just setting it
> state->ignore_not_found = true; in sysdb_delete_entry_send().
> 
> I think the correct thing to do would be to modify that function so that
> it accepted a boolean value for whether or not to ignore it if it's not
> found.
> 
> This way, we aren't doing an extra search.

I agree, I didn't realize there was this option. Thanks for the review.

Attached are three patches:
* [PATCH 1/3] Add ignore_not_found parameter to sysdb delete functions
changes the sysdb API, its invocations in the code and adds a test for
the change
* [PATCH 2/3] Use correct return codes
The already-acked patch, included for clarity
* [PATCH 3/3] Notify user when deleting nonexistent user or group
Changes tools code to react with appropriate error message to ENOENT

        Jakub
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (GNU/Linux)
Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org/

iEYEARECAAYFAkpywRkACgkQHsardTLnvCXZxACgpoTd+OQHRec9YpzR+JQH+8SN
FlgAoNVZM838bstekX49AdxE/X2iCOUH
=tMJo
-----END PGP SIGNATURE-----
>From ef7bc5a7762f6437cd18a3d81042dadffc92c8b9 Mon Sep 17 00:00:00 2001
From: Jakub Hrozek <jhro...@redhat.com>
Date: Thu, 30 Jul 2009 23:25:03 +0200
Subject: [PATCH 1/3] Add ignore_not_found parameter to sysdb delete functions

Also add tests
---
 server/db/sysdb.h          |    9 ++-
 server/db/sysdb_ops.c      |   23 ++++--
 server/providers/proxy.c   |   15 +++--
 server/tests/sysdb-tests.c |  156 ++++++++++++++++++++++++++++++++++++++++++-
 4 files changed, 182 insertions(+), 21 deletions(-)

diff --git a/server/db/sysdb.h b/server/db/sysdb.h
index d6ecb2a..df2a946 100644
--- a/server/db/sysdb.h
+++ b/server/db/sysdb.h
@@ -274,7 +274,8 @@ int sysdb_get_user_attr(TALLOC_CTX *mem_ctx,
 struct tevent_req *sysdb_delete_entry_send(TALLOC_CTX *mem_ctx,
                                            struct tevent_context *ev,
                                            struct sysdb_handle *handle,
-                                           struct ldb_dn *dn);
+                                           struct ldb_dn *dn,
+                                           bool ignore_not_found);
 int sysdb_delete_entry_recv(struct tevent_req *req);
 
 /* Search Entry */
@@ -313,7 +314,8 @@ struct tevent_req *sysdb_delete_user_by_uid_send(TALLOC_CTX *mem_ctx,
                                                  struct tevent_context *ev,
                                                  struct sysdb_handle *handle,
                                                  struct sss_domain_info *domain,
-                                                 uid_t uid);
+                                                 uid_t uid,
+                                                 bool ignore_not_found);
 int sysdb_delete_user_by_uid_recv(struct tevent_req *req);
 
 /* Search Group (gy gid or name) */
@@ -340,7 +342,8 @@ struct tevent_req *sysdb_delete_group_by_gid_send(TALLOC_CTX *mem_ctx,
                                                   struct tevent_context *ev,
                                                   struct sysdb_handle *handle,
                                                   struct sss_domain_info *domain,
-                                                  gid_t gid);
+                                                  gid_t gid,
+                                                  bool ignore_not_found);
 int sysdb_delete_group_by_gid_recv(struct tevent_req *req);
 
 /* Replace entry attrs */
diff --git a/server/db/sysdb_ops.c b/server/db/sysdb_ops.c
index c172b70..8610e63 100644
--- a/server/db/sysdb_ops.c
+++ b/server/db/sysdb_ops.c
@@ -245,7 +245,8 @@ static int sysdb_op_default_recv(struct tevent_req *req)
 struct tevent_req *sysdb_delete_entry_send(TALLOC_CTX *mem_ctx,
                                            struct tevent_context *ev,
                                            struct sysdb_handle *handle,
-                                           struct ldb_dn *dn)
+                                           struct ldb_dn *dn,
+                                           bool ignore_not_found)
 {
     struct tevent_req *req, *subreq;
     struct sysdb_op_state *state;
@@ -257,7 +258,7 @@ struct tevent_req *sysdb_delete_entry_send(TALLOC_CTX *mem_ctx,
 
     state->ev = ev;
     state->handle = handle;
-    state->ignore_not_found = true;
+    state->ignore_not_found = ignore_not_found;
     state->ldbreply = NULL;
 
     ret = ldb_build_del_req(&ldbreq, handle->ctx->ldb, state, dn,
@@ -613,7 +614,8 @@ struct tevent_req *sysdb_delete_user_by_uid_send(TALLOC_CTX *mem_ctx,
                                                  struct tevent_context *ev,
                                                  struct sysdb_handle *handle,
                                                  struct sss_domain_info *domain,
-                                                 uid_t uid)
+                                                 uid_t uid,
+                                                 bool ignore_not_found)
 {
     struct tevent_req *req, *subreq;
     struct sysdb_op_state *state;
@@ -623,7 +625,7 @@ struct tevent_req *sysdb_delete_user_by_uid_send(TALLOC_CTX *mem_ctx,
 
     state->ev = ev;
     state->handle = handle;
-    state->ignore_not_found = true;
+    state->ignore_not_found = ignore_not_found;
     state->ldbreply = NULL;
 
     subreq = sysdb_search_user_by_uid_send(state, ev, NULL, handle,
@@ -656,7 +658,9 @@ static void sysdb_delete_user_by_uid_found(struct tevent_req *subreq)
         return;
     }
 
-    subreq = sysdb_delete_entry_send(state, state->ev, state->handle, msg->dn);
+    subreq = sysdb_delete_entry_send(state, state->ev,
+                                     state->handle, msg->dn,
+                                     state->ignore_not_found);
     if (!subreq) {
         tevent_req_error(req, ENOMEM);
         return;
@@ -896,7 +900,8 @@ struct tevent_req *sysdb_delete_group_by_gid_send(TALLOC_CTX *mem_ctx,
                                                   struct tevent_context *ev,
                                                   struct sysdb_handle *handle,
                                                   struct sss_domain_info *domain,
-                                                  gid_t gid)
+                                                  gid_t gid,
+                                                  bool ignore_not_found)
 {
     struct tevent_req *req, *subreq;
     struct sysdb_op_state *state;
@@ -906,7 +911,7 @@ struct tevent_req *sysdb_delete_group_by_gid_send(TALLOC_CTX *mem_ctx,
 
     state->ev = ev;
     state->handle = handle;
-    state->ignore_not_found = true;
+    state->ignore_not_found = ignore_not_found;
     state->ldbreply = NULL;
 
     subreq = sysdb_search_group_by_gid_send(state, ev, NULL, handle,
@@ -939,7 +944,9 @@ static void sysdb_delete_group_by_gid_found(struct tevent_req *subreq)
         return;
     }
 
-    subreq = sysdb_delete_entry_send(state, state->ev, state->handle, msg->dn);
+    subreq = sysdb_delete_entry_send(state, state->ev,
+                                     state->handle, msg->dn,
+                                     state->ignore_not_found);
     if (!subreq) {
         tevent_req_error(req, ENOMEM);
         return;
diff --git a/server/providers/proxy.c b/server/providers/proxy.c
index ddfbe39..106530f 100644
--- a/server/providers/proxy.c
+++ b/server/providers/proxy.c
@@ -477,7 +477,7 @@ static void get_pw_name_process(struct tevent_req *subreq)
             return;
         }
 
-        subreq = sysdb_delete_entry_send(state, state->ev, state->handle, dn);
+        subreq = sysdb_delete_entry_send(state, state->ev, state->handle, dn, true);
         if (!subreq) {
             tevent_req_error(req, ENOMEM);
             return;
@@ -650,7 +650,8 @@ static void get_pw_uid_process(struct tevent_req *subreq)
         subreq = sysdb_delete_user_by_uid_send(state, state->ev,
                                                state->handle,
                                                state->domain,
-                                               state->uid);
+                                               state->uid,
+                                               true);
         if (!subreq) {
             tevent_req_error(req, ENOMEM);
             return;
@@ -979,7 +980,7 @@ again:
             return;
         }
 
-        subreq = sysdb_delete_entry_send(state, state->ev, state->handle, dn);
+        subreq = sysdb_delete_entry_send(state, state->ev, state->handle, dn, true);
         if (!subreq) {
             tevent_req_error(req, ENOMEM);
             return;
@@ -1166,7 +1167,8 @@ again:
         subreq = sysdb_delete_group_by_gid_send(state, state->ev,
                                                 state->handle,
                                                 state->domain,
-                                                state->gid);
+                                                state->gid,
+                                                true);
         if (!subreq) {
             tevent_req_error(req, ENOMEM);
             return;
@@ -1496,7 +1498,7 @@ static void get_initgr_process(struct tevent_req *subreq)
             return;
         }
 
-        subreq = sysdb_delete_entry_send(state, state->ev, state->handle, dn);
+        subreq = sysdb_delete_entry_send(state, state->ev, state->handle, dn, true);
         if (!subreq) {
             tevent_req_error(req, ENOMEM);
             return;
@@ -1797,7 +1799,8 @@ again:
         subreq = sysdb_delete_group_by_gid_send(state, state->ev,
                                                 state->handle,
                                                 state->domain,
-                                                state->gid);
+                                                state->gid,
+                                                true);
         if (!subreq) {
             ret = ENOMEM;
             goto fail;
diff --git a/server/tests/sysdb-tests.c b/server/tests/sysdb-tests.c
index b2dc8ab..72f80ed 100644
--- a/server/tests/sysdb-tests.c
+++ b/server/tests/sysdb-tests.c
@@ -313,7 +313,7 @@ static void test_remove_user(struct tevent_req *req)
     user_dn = sysdb_user_dn(data->ctx->sysdb, data, "LOCAL", data->username);
     if (!user_dn) return test_return(data, ENOMEM);
 
-    subreq = sysdb_delete_entry_send(data, data->ev, data->handle, user_dn);
+    subreq = sysdb_delete_entry_send(data, data->ev, data->handle, user_dn, true);
     if (!subreq) return test_return(data, ENOMEM);
 
     tevent_req_set_callback(subreq, test_remove_user_done, data);
@@ -346,7 +346,8 @@ static void test_remove_user_by_uid(struct tevent_req *req)
 
     subreq = sysdb_delete_user_by_uid_send(data,
                                            data->ev, data->handle,
-                                           data->domain, data->uid);
+                                           data->domain, data->uid,
+                                           true);
     if (!subreq) return test_return(data, ENOMEM);
 
     tevent_req_set_callback(subreq, test_remove_user_by_uid_done, data);
@@ -364,6 +365,74 @@ static void test_remove_user_by_uid_done(struct tevent_req *subreq)
     return test_return(data, ret);
 }
 
+static void test_remove_nonexistent_group_done(struct tevent_req *subreq);
+
+static void test_remove_nonexistent_group(struct tevent_req *req)
+{
+    struct test_data *data = tevent_req_callback_data(req, struct test_data);
+    struct tevent_req *subreq;
+    int ret;
+
+    ret = sysdb_transaction_recv(req, data, &data->handle);
+    if (ret != EOK) {
+        return test_return(data, ret);
+    }
+
+    subreq = sysdb_delete_group_by_gid_send(data,
+                                           data->ev, data->handle,
+                                           data->domain, data->uid,
+                                           false);
+    if (!subreq) return test_return(data, ENOMEM);
+
+    tevent_req_set_callback(subreq, test_remove_nonexistent_group_done, data);
+}
+
+static void test_remove_nonexistent_group_done(struct tevent_req *subreq)
+{
+    struct test_data *data = tevent_req_callback_data(subreq,
+                                                      struct test_data);
+    int ret;
+
+    ret = sysdb_delete_group_by_gid_recv(subreq);
+    talloc_zfree(subreq);
+
+    return test_return(data, ret);
+}
+
+static void test_remove_nonexistent_user_done(struct tevent_req *subreq);
+
+static void test_remove_nonexistent_user(struct tevent_req *req)
+{
+    struct test_data *data = tevent_req_callback_data(req, struct test_data);
+    struct tevent_req *subreq;
+    int ret;
+
+    ret = sysdb_transaction_recv(req, data, &data->handle);
+    if (ret != EOK) {
+        return test_return(data, ret);
+    }
+
+    subreq = sysdb_delete_user_by_uid_send(data,
+                                           data->ev, data->handle,
+                                           data->domain, data->uid,
+                                           false);
+    if (!subreq) return test_return(data, ENOMEM);
+
+    tevent_req_set_callback(subreq, test_remove_nonexistent_user_done, data);
+}
+
+static void test_remove_nonexistent_user_done(struct tevent_req *subreq)
+{
+    struct test_data *data = tevent_req_callback_data(subreq,
+                                                      struct test_data);
+    int ret;
+
+    ret = sysdb_delete_user_by_uid_recv(subreq);
+    talloc_zfree(subreq);
+
+    return test_return(data, ret);
+}
+
 static void test_add_group_done(struct tevent_req *subreq);
 
 static void test_add_group(struct tevent_req *req)
@@ -448,7 +517,7 @@ static void test_remove_group(struct tevent_req *req)
     group_dn = sysdb_group_dn(data->ctx->sysdb, data, "LOCAL", data->groupname);
     if (!group_dn) return test_return(data, ENOMEM);
 
-    subreq = sysdb_delete_entry_send(data, data->ev, data->handle, group_dn);
+    subreq = sysdb_delete_entry_send(data, data->ev, data->handle, group_dn, true);
     if (!subreq) return test_return(data, ENOMEM);
 
     tevent_req_set_callback(subreq, test_remove_group_done, data);
@@ -479,7 +548,8 @@ static void test_remove_group_by_gid(struct tevent_req *req)
     }
 
     subreq = sysdb_delete_group_by_gid_send(data, data->ev, data->handle,
-                                            data->domain, data->gid);
+                                            data->domain, data->gid,
+                                            true);
     if (!subreq) return test_return(data, ENOMEM);
 
     tevent_req_set_callback(subreq, test_remove_group_by_gid_done, data);
@@ -1478,6 +1548,78 @@ START_TEST (test_sysdb_remove_group_member)
 }
 END_TEST
 
+START_TEST (test_sysdb_remove_nonexistent_user)
+{
+    struct sysdb_test_ctx *test_ctx;
+    struct test_data *data;
+    struct tevent_req *req;
+    int ret;
+
+    /* Setup */
+    ret = setup_sysdb_tests(&test_ctx);
+    if (ret != EOK) {
+        fail("Could not set up the test");
+        return;
+    }
+
+    data = talloc_zero(test_ctx, struct test_data);
+    data->ctx = test_ctx;
+    data->ev = test_ctx->ev;
+    data->uid = 12345;
+    data->domain = get_local_domain(test_ctx->domains);
+
+    req = sysdb_transaction_send(data, data->ev, test_ctx->sysdb);
+    if (!req) {
+        ret = ENOMEM;
+    }
+
+    if (ret == EOK) {
+        tevent_req_set_callback(req, test_remove_nonexistent_user, data);
+
+        ret = test_loop(data);
+    }
+
+    fail_if(ret != ENOENT, "Unexpected return code %d, expected ENOENT", ret);
+    talloc_free(test_ctx);
+}
+END_TEST
+
+START_TEST (test_sysdb_remove_nonexistent_group)
+{
+    struct sysdb_test_ctx *test_ctx;
+    struct test_data *data;
+    struct tevent_req *req;
+    int ret;
+
+    /* Setup */
+    ret = setup_sysdb_tests(&test_ctx);
+    if (ret != EOK) {
+        fail("Could not set up the test");
+        return;
+    }
+
+    data = talloc_zero(test_ctx, struct test_data);
+    data->ctx = test_ctx;
+    data->ev = test_ctx->ev;
+    data->uid = 12345;
+    data->domain = get_local_domain(test_ctx->domains);
+
+    req = sysdb_transaction_send(data, data->ev, test_ctx->sysdb);
+    if (!req) {
+        ret = ENOMEM;
+    }
+
+    if (ret == EOK) {
+        tevent_req_set_callback(req, test_remove_nonexistent_group, data);
+
+        ret = test_loop(data);
+    }
+
+    fail_if(ret != ENOENT, "Unexpected return code %d, expected ENOENT", ret);
+    talloc_free(test_ctx);
+}
+END_TEST
+
 Suite *create_sysdb_suite(void)
 {
     Suite *s = suite_create("sysdb");
@@ -1544,6 +1686,12 @@ Suite *create_sysdb_suite(void)
     /* Remove the groups by name */
     tcase_add_loop_test(tc_sysdb, test_sysdb_remove_local_group, 28010, 28020);
 
+    /* test the ignore_not_found parameter for users */
+    tcase_add_test(tc_sysdb, test_sysdb_remove_nonexistent_user);
+
+    /* test the ignore_not_found parameter for groups */
+    tcase_add_test(tc_sysdb, test_sysdb_remove_nonexistent_group);
+
 /* Add all test cases to the test suite */
     suite_add_tcase(s, tc_sysdb);
 
-- 
1.6.2.5

>From d774a7c2bd236eddcab69b24b8efde31397100e8 Mon Sep 17 00:00:00 2001
From: Jakub Hrozek <jhro...@redhat.com>
Date: Thu, 30 Jul 2009 18:03:47 +0200
Subject: [PATCH 2/3] Use correct return codes

Some code paths that should exit with an error used potentionally
incorrect return code.
---
 server/tools/sss_groupdel.c |    2 +-
 server/tools/sss_groupmod.c |    2 +-
 server/tools/sss_userdel.c  |    2 +-
 server/tools/sss_usermod.c  |    2 +-
 4 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/server/tools/sss_groupdel.c b/server/tools/sss_groupdel.c
index 98d73c3..2977826 100644
--- a/server/tools/sss_groupdel.c
+++ b/server/tools/sss_groupdel.c
@@ -99,7 +99,7 @@ static void group_del(struct tevent_req *req)
 
     subreq = sysdb_delete_entry_send(data, data->ev, data->handle, group_dn);
     if (!subreq)
-        return groupdel_done(data, ret, NULL);
+        return groupdel_done(data, ENOMEM, NULL);
 
     tevent_req_set_callback(subreq, group_del_done, data);
 }
diff --git a/server/tools/sss_groupmod.c b/server/tools/sss_groupmod.c
index 2fc6f9d..a7fec43 100644
--- a/server/tools/sss_groupmod.c
+++ b/server/tools/sss_groupmod.c
@@ -114,7 +114,7 @@ static void mod_group(struct tevent_req *req)
                                            data->domain, data->name,
                                            attrs, SYSDB_MOD_REP);
         if (!subreq) {
-            return mod_group_done(data, ret);
+            return mod_group_done(data, ENOMEM);
         }
         tevent_req_set_callback(subreq, mod_group_attr_done, data);
         return;
diff --git a/server/tools/sss_userdel.c b/server/tools/sss_userdel.c
index f70482c..c4d9abd 100644
--- a/server/tools/sss_userdel.c
+++ b/server/tools/sss_userdel.c
@@ -100,7 +100,7 @@ static void user_del(struct tevent_req *req)
 
     subreq = sysdb_delete_entry_send(data, data->ev, data->handle, user_dn);
     if (!subreq)
-        return userdel_done(data, ret, NULL);
+        return userdel_done(data, ENOMEM, NULL);
 
     tevent_req_set_callback(subreq, user_del_done, data);
 }
diff --git a/server/tools/sss_usermod.c b/server/tools/sss_usermod.c
index 6ef873b..7237f0b 100644
--- a/server/tools/sss_usermod.c
+++ b/server/tools/sss_usermod.c
@@ -140,7 +140,7 @@ static void mod_user(struct tevent_req *req)
                                           data->domain, data->name,
                                           data->attrs, SYSDB_MOD_REP);
         if (!subreq) {
-            return mod_user_done(data, ret);
+            return mod_user_done(data, ENOMEM);
         }
         tevent_req_set_callback(subreq, mod_user_attr_done, data);
         return;
-- 
1.6.2.5

>From 6b9f651308fd16a38af7f7eeb34b80108f31e974 Mon Sep 17 00:00:00 2001
From: Jakub Hrozek <jhro...@redhat.com>
Date: Thu, 30 Jul 2009 11:40:08 +0200
Subject: [PATCH 3/3] Notify user when deleting nonexistent user or group

Fixes: RHBZ #513247, RHBZ #513250
---
 server/tools/sss_groupdel.c |   12 ++++++++++--
 server/tools/sss_userdel.c  |   13 ++++++++++---
 2 files changed, 20 insertions(+), 5 deletions(-)

diff --git a/server/tools/sss_groupdel.c b/server/tools/sss_groupdel.c
index 2977826..a8b9357 100644
--- a/server/tools/sss_groupdel.c
+++ b/server/tools/sss_groupdel.c
@@ -97,7 +97,7 @@ static void group_del(struct tevent_req *req)
         return groupdel_done(data, ENOMEM, NULL);
     }
 
-    subreq = sysdb_delete_entry_send(data, data->ev, data->handle, group_dn);
+    subreq = sysdb_delete_entry_send(data, data->ev, data->handle, group_dn, false);
     if (!subreq)
         return groupdel_done(data, ENOMEM, NULL);
 
@@ -255,7 +255,15 @@ int main(int argc, const char **argv)
     if (data->error) {
         ret = data->error;
         DEBUG(1, ("sysdb operation failed (%d)[%s]\n", ret, strerror(ret)));
-        ERROR("Transaction error. Could not remove group.\n");
+        switch (ret) {
+            case ENOENT:
+                ERROR("No such group\n");
+                break;
+
+            default:
+                ERROR("Internal error. Could not remove group.\n");
+                break;
+        }
         ret = EXIT_FAILURE;
         goto fini;
     }
diff --git a/server/tools/sss_userdel.c b/server/tools/sss_userdel.c
index c4d9abd..f7cb757 100644
--- a/server/tools/sss_userdel.c
+++ b/server/tools/sss_userdel.c
@@ -98,7 +98,7 @@ static void user_del(struct tevent_req *req)
         return userdel_done(data, ENOMEM, NULL);
     }
 
-    subreq = sysdb_delete_entry_send(data, data->ev, data->handle, user_dn);
+    subreq = sysdb_delete_entry_send(data, data->ev, data->handle, user_dn, false);
     if (!subreq)
         return userdel_done(data, ENOMEM, NULL);
 
@@ -115,7 +115,6 @@ static void user_del_done(struct tevent_req *subreq)
     return userdel_done(data, ret, NULL);
 }
 
-
 static int userdel_legacy(struct ops_ctx *ctx)
 {
     int ret = EOK;
@@ -257,7 +256,15 @@ int main(int argc, const char **argv)
     if (data->error) {
         ret = data->error;
         DEBUG(1, ("sysdb operation failed (%d)[%s]\n", ret, strerror(ret)));
-        ERROR("Internal error. Could not remove user.\n");
+        switch (ret) {
+            case ENOENT:
+                ERROR("No such user\n");
+                break;
+
+            default:
+                ERROR("Internal error. Could not remove user.\n");
+                break;
+        }
         ret = EXIT_FAILURE;
         goto fini;
     }
-- 
1.6.2.5

_______________________________________________
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel

Reply via email to