Here are two patches for the SYSDB:
[PATCH 1/2] SYSDB: replace ghost users properly
this fixes https://fedorahosted.org/sssd/ticket/1714
[PATCH 2/2] SYSDB: split sysdb_add_user
splits add_user code to more readable blocks
Patches are attached
Ondra
On Thu, Dec 20, 2012 at 02:05:38PM +0100, Ondrej Kos wrote:
Here are two patches for the SYSDB:
[PATCH 1/2] SYSDB: replace ghost users properly
this fixes https://fedorahosted.org/sssd/ticket/1714
[PATCH 2/2] SYSDB: split sysdb_add_user
splits add_user code to more readable blocks
Patches are attached
I haven't tested the patches yet, just by visual inspection:
From 54ed4f83cd28eb8a08a7f402847b22057f0a10f6 Mon Sep 17 00:00:00 2001 From: Ondrej Kos okos@redhat.com Date: Thu, 20 Dec 2012 13:15:31 +0100 Subject: [PATCH 1/2] SYSDB: replace ghost users properly
https://fedorahosted.org/sssd/ticket/1714
We were trying to delete all existing ghosts for users name and alias even if they weren't in the database. Now only the matching entries are removed and replaced by member.
src/db/sysdb_ops.c | 41 ++++++++++++++++++++++++++++++----------- 1 file changed, 30 insertions(+), 11 deletions(-)
diff --git a/src/db/sysdb_ops.c b/src/db/sysdb_ops.c index 4bbc76c5c39501a4d878a1d41faa81919a23228f..fbd5c413581a72240b798ba68123efef8d470f09 100644 --- a/src/db/sysdb_ops.c +++ b/src/db/sysdb_ops.c @@ -74,6 +74,22 @@ static uint32_t get_attr_as_uint32(struct ldb_message *msg, const char *attr) return l; }
+static int find_name_in_ghosts(const char *name,
struct ldb_message_element *ghosts)
This function sounds like it should have been using bool?
+{
- int ret = 0;
- int i;
- for (i = 0; i < ghosts->num_values; i++) {
if (strcmp(name, (char *) ghosts->values[i].data)) {ret = i;break;}- }
- return ret;
+}
#define ERROR_OUT(v, r, l) do { v = r; goto l; } while(0)
@@ -879,6 +895,7 @@ int sysdb_add_user(struct sysdb_ctx *sysdb, uint32_t id; int ret, i, j; bool add_member = false;
struct ldb_message_element *ghosts;
struct sss_domain_info *domain = sysdb->domain;
@@ -1081,18 +1098,20 @@ int sysdb_add_user(struct sysdb_ctx *sysdb, if (ret) goto done; }
ret = add_string(msg, LDB_FLAG_MOD_DELETE, SYSDB_GHOST, name);if (ret) goto done;
ghosts = NULL;ghosts = ldb_msg_find_element(groups[i], SYSDB_GHOST);
/* Delete aliases from the ghost attribute as well */for (j = 0; j < alias_el->num_values; j++) {if (strcmp((const char *)alias_el->values[j].data, name) == 0) {continue;}ret = ldb_msg_add_string(msg, SYSDB_GHOST,(char *) alias_el->values[j].data);if (ret != LDB_SUCCESS) {ERROR_OUT(ret, EINVAL, done);
if (ghosts != NULL) {if (find_name_in_ghosts(name, ghosts) != 0){
Code style, don't open with { on a newline
ret = add_string(msg, LDB_FLAG_MOD_DELETE, SYSDB_GHOST, name);} else {for (j = 0; j < alias_el->num_values; j++) {if (find_name_in_ghosts((const char *)alias_el->values[j].data, ghosts) != 0) {ret = add_string(msg, LDB_FLAG_MOD_DELETE, SYSDB_GHOST, (char *)alias_el->values[j].data);break;
I wonder if there could be a group that would reference a single user with both name and alias. Then we would probably only remove one alias in this code. The group would be broken, but we should be defensive.
}} } }-- 1.7.11.7
On Thu, 2012-12-20 at 14:05 +0100, Ondrej Kos wrote:
Here are two patches for the SYSDB:
[PATCH 1/2] SYSDB: replace ghost users properly
this fixes https://fedorahosted.org/sssd/ticket/1714
It seem that here you are basically scanning the whole attribute. Instead of making potentially multiple deletes then wouldn't it be easier to recompute the attribute and do a single replace omitting the names you do not want in there? I think you can significantly reduce the amount of linear searches as the current patch introduces O(n^2) like behavior (nested linear loops).
Simo.
[PATCH 2/2] SYSDB: split sysdb_add_user
splits add_user code to more readable blocks
Patches are attached
On Thu, 2012-12-20 at 08:28 -0500, Simo Sorce wrote:
On Thu, 2012-12-20 at 14:05 +0100, Ondrej Kos wrote:
Here are two patches for the SYSDB:
[PATCH 1/2] SYSDB: replace ghost users properly
this fixes https://fedorahosted.org/sssd/ticket/1714
It seem that here you are basically scanning the whole attribute. Instead of making potentially multiple deletes then wouldn't it be easier to recompute the attribute and do a single replace omitting the names you do not want in there? I think you can significantly reduce the amount of linear searches as the current patch introduces O(n^2) like behavior (nested linear loops).
Ok scrap this suggestion, I did not understand the patch and Ondrej explained it to me in private.
However now that I understood what problem we are trying to solve here I am afraid this work can be simply doen with adding a control named LDB_CONTROL_PERMISSIVE_MODIFY_OID available in ldb by default.
It will cause a deletion of a missing value to not return errors.
Simo.
On Fri, Jan 04, 2013 at 09:13:04AM -0500, Simo Sorce wrote:
On Thu, 2012-12-20 at 08:28 -0500, Simo Sorce wrote:
On Thu, 2012-12-20 at 14:05 +0100, Ondrej Kos wrote:
Here are two patches for the SYSDB:
[PATCH 1/2] SYSDB: replace ghost users properly
this fixes https://fedorahosted.org/sssd/ticket/1714
It seem that here you are basically scanning the whole attribute. Instead of making potentially multiple deletes then wouldn't it be easier to recompute the attribute and do a single replace omitting the names you do not want in there? I think you can significantly reduce the amount of linear searches as the current patch introduces O(n^2) like behavior (nested linear loops).
Ok scrap this suggestion, I did not understand the patch and Ondrej explained it to me in private.
However now that I understood what problem we are trying to solve here I am afraid this work can be simply doen with adding a control named LDB_CONTROL_PERMISSIVE_MODIFY_OID available in ldb by default.
It will cause a deletion of a missing value to not return errors.
You mean something like Samba had in dsdb_modify_permissive()?
In pseudocode it said:
int modify_permissive(ldb, msg) { ldb_request req;
ldb_build_mod_req(&req, ldb, msg);
ldb_request_add_control(req, LDB_CONTROL_PERMISSIVE_MODIFY_OID, false, NULL);
ret = ldb_request(ldb, req); if (ret == LDB_SUCCESS) { ret = ldb_wait(req->handle, LDB_WAIT_ALL); } }
I'm a little concerned that this approach would hide genuine logic bugs, though..but for a low level function such as sysdb_add_user that is called a lot, this might be OK for performance reasons..
On Fri, 2013-01-04 at 16:15 +0100, Jakub Hrozek wrote:
On Fri, Jan 04, 2013 at 09:13:04AM -0500, Simo Sorce wrote:
On Thu, 2012-12-20 at 08:28 -0500, Simo Sorce wrote:
On Thu, 2012-12-20 at 14:05 +0100, Ondrej Kos wrote:
Here are two patches for the SYSDB:
[PATCH 1/2] SYSDB: replace ghost users properly
this fixes https://fedorahosted.org/sssd/ticket/1714
It seem that here you are basically scanning the whole attribute. Instead of making potentially multiple deletes then wouldn't it be easier to recompute the attribute and do a single replace omitting the names you do not want in there? I think you can significantly reduce the amount of linear searches as the current patch introduces O(n^2) like behavior (nested linear loops).
Ok scrap this suggestion, I did not understand the patch and Ondrej explained it to me in private.
However now that I understood what problem we are trying to solve here I am afraid this work can be simply doen with adding a control named LDB_CONTROL_PERMISSIVE_MODIFY_OID available in ldb by default.
It will cause a deletion of a missing value to not return errors.
You mean something like Samba had in dsdb_modify_permissive()?
In pseudocode it said:
int modify_permissive(ldb, msg) { ldb_request req;
ldb_build_mod_req(&req, ldb, msg); ldb_request_add_control(req, LDB_CONTROL_PERMISSIVE_MODIFY_OID, false, NULL); ret = ldb_request(ldb, req); if (ret == LDB_SUCCESS) { ret = ldb_wait(req->handle, LDB_WAIT_ALL); }}
Yes, I asked Ondrej in fact to build a sss_permissive_modify() function. This is a perfect template.
I'm a little concerned that this approach would hide genuine logic bugs, though..but for a low level function such as sysdb_add_user that is called a lot, this might be OK for performance reasons..
I think the compromise is justified, we also do create users in steps so this modify would affect only a subset of attributes.
Simo.
On 04/01/13 18:40, Simo Sorce wrote:
On Fri, 2013-01-04 at 16:15 +0100, Jakub Hrozek wrote:
On Fri, Jan 04, 2013 at 09:13:04AM -0500, Simo Sorce wrote:
On Thu, 2012-12-20 at 08:28 -0500, Simo Sorce wrote:
On Thu, 2012-12-20 at 14:05 +0100, Ondrej Kos wrote:
Here are two patches for the SYSDB:
[PATCH 1/2] SYSDB: replace ghost users properly
this fixes https://fedorahosted.org/sssd/ticket/1714
It seem that here you are basically scanning the whole attribute. Instead of making potentially multiple deletes then wouldn't it be easier to recompute the attribute and do a single replace omitting the names you do not want in there? I think you can significantly reduce the amount of linear searches as the current patch introduces O(n^2) like behavior (nested linear loops).
Ok scrap this suggestion, I did not understand the patch and Ondrej explained it to me in private.
However now that I understood what problem we are trying to solve here I am afraid this work can be simply doen with adding a control named LDB_CONTROL_PERMISSIVE_MODIFY_OID available in ldb by default.
It will cause a deletion of a missing value to not return errors.
You mean something like Samba had in dsdb_modify_permissive()?
In pseudocode it said:
int modify_permissive(ldb, msg) { ldb_request req;
ldb_build_mod_req(&req, ldb, msg); ldb_request_add_control(req, LDB_CONTROL_PERMISSIVE_MODIFY_OID, false, NULL); ret = ldb_request(ldb, req); if (ret == LDB_SUCCESS) { ret = ldb_wait(req->handle, LDB_WAIT_ALL); }}
Yes, I asked Ondrej in fact to build a sss_permissive_modify() function. This is a perfect template.
I'm a little concerned that this approach would hide genuine logic bugs, though..but for a low level function such as sysdb_add_user that is called a lot, this might be OK for performance reasons..
I think the compromise is justified, we also do create users in steps so this modify would affect only a subset of attributes.
Simo.
New patches are attached, the permissive mode works fine.
Ondra
On Mon, 2013-01-07 at 15:01 +0100, Ondrej Kos wrote:
tps://fedorahosted.org/sssd/ticket/1714
The attempt to delete all ghosts for users name and aliases was failing, resulting into failure of whole user-add operation. In permissive mode, the attempts to delete non-existent entries are not interpreted as error.
Comments inline.
diff --git a/src/db/sysdb_ops.c b/src/db/sysdb_ops.c index 4bbc76c5c39501a4d878a1d41faa81919a23228f..6fb9b1d1e17f15710e3e1cff6577865525391699 100644 --- a/src/db/sysdb_ops.c +++ b/src/db/sysdb_ops.c @@ -74,6 +74,38 @@ static uint32_t get_attr_as_uint32(struct ldb_message *msg, const char *attr) return l; }
+errno_t +sss_ldb_modify_permissive(struct ldb_context *ldb,
struct ldb_message *msg)
Do not \n after errno_t Make it a static function. Properly align the second argument. You are not returing errno_t here you are returning LDB errors, so just use 'int'.
+{
- struct ldb_request *req;
- errno_t ret = EOK;
'int' not errno_t here.
Whole indent one tab too much after this line.
ret = ldb_build_mod_req(&req, ldb, ldb,msg,NULL,NULL,ldb_op_default_callback,NULL);
Also misaligned arguments.
if (ret != LDB_SUCCESS) return ret;ret = ldb_request_add_control(req,LDB_CONTROL_PERMISSIVE_MODIFY_OID, false, NULL);
Split this line so iut doesn't wrap on 80 please
if (ret != LDB_SUCCESS) {talloc_free(req);return ret;}
Indent issue here too in ineer block.
ret = ldb_request(ldb, req);if (ret == LDB_SUCCESS) {ret = ldb_wait(req->handle, LDB_WAIT_ALL);}
Again indent in inner block.
talloc_free(req);- return ret;
+}
Yeah copy&paste from samba code is not straightforward :-)
Once you fix these issues you get an ACK.
Simo.
On 07/01/13 15:08, Simo Sorce wrote:
On Mon, 2013-01-07 at 15:01 +0100, Ondrej Kos wrote:
tps://fedorahosted.org/sssd/ticket/1714
The attempt to delete all ghosts for users name and aliases was failing, resulting into failure of whole user-add operation. In permissive mode, the attempts to delete non-existent entries are not interpreted as error.
Comments inline.
diff --git a/src/db/sysdb_ops.c b/src/db/sysdb_ops.c index 4bbc76c5c39501a4d878a1d41faa81919a23228f..6fb9b1d1e17f15710e3e1cff6577865525391699 100644 --- a/src/db/sysdb_ops.c +++ b/src/db/sysdb_ops.c @@ -74,6 +74,38 @@ static uint32_t get_attr_as_uint32(struct ldb_message *msg, const char *attr) return l; }
+errno_t +sss_ldb_modify_permissive(struct ldb_context *ldb,
struct ldb_message *msg)Do not \n after errno_t Make it a static function. Properly align the second argument. You are not returing errno_t here you are returning LDB errors, so just use 'int'.
+{
- struct ldb_request *req;
- errno_t ret = EOK;
'int' not errno_t here.
Whole indent one tab too much after this line.
ret = ldb_build_mod_req(&req, ldb, ldb,msg,NULL,NULL,ldb_op_default_callback,NULL);Also misaligned arguments.
if (ret != LDB_SUCCESS) return ret;ret = ldb_request_add_control(req,LDB_CONTROL_PERMISSIVE_MODIFY_OID, false, NULL);
Split this line so iut doesn't wrap on 80 please
if (ret != LDB_SUCCESS) {talloc_free(req);return ret;}Indent issue here too in ineer block.
ret = ldb_request(ldb, req);if (ret == LDB_SUCCESS) {ret = ldb_wait(req->handle, LDB_WAIT_ALL);}Again indent in inner block.
talloc_free(req);- return ret;
+}
Yeah copy&paste from samba code is not straightforward :-)
Once you fix these issues you get an ACK.
Simo.
Thanks for the review, the indentation looked fine in vim, however my script somehow misbehaved and haven't replaced tab's with spaces.
New patches are attached
Ondra
On Mon, Jan 07, 2013 at 09:53:38AM -0500, Simo Sorce wrote:
On Mon, 2013-01-07 at 15:24 +0100, Ondrej Kos wrote:
Thanks for the review, the indentation looked fine in vim, however my script somehow misbehaved and haven't replaced tab's with spaces.
New patches are attached
ACK.
Simo.
Patch #1 pushed to master and sssd-1-9
Patch #2 pushed to master
sssd-devel@lists.fedorahosted.org
-
Jakub Hrozek -
Ondrej Kos -
Simo Sorce