[SSSD] [PATCH] SYSDB: Return errno, not ldb error code

Jakub Hrozek jhrozek at redhat.com
Mon Aug 18 06:31:38 UTC 2014


On Mon, Aug 18, 2014 at 08:11:29AM +0200, Lukas Slebodnik wrote:
> On (17/08/14 20:45), Jakub Hrozek wrote:
> >Hi,
> >
> >for one of my patches I tried to use ldb permissive modification before
> >realising it's not the best approach. At least I found this little bug
> >in the sysdb code.
> 
> >From de9f9376b6d5ed098a268a7a1f5d04bca1c8d96f Mon Sep 17 00:00:00 2001
> >From: Jakub Hrozek <jhrozek at redhat.com>
> >Date: Sat, 16 Aug 2014 19:41:40 +0200
> >Subject: [PATCH] SYSDB: Return errno, not ldb error code
> >
> >---
> > src/db/sysdb_ops.c | 3 +++
> > 1 file changed, 3 insertions(+)
> >
> >diff --git a/src/db/sysdb_ops.c b/src/db/sysdb_ops.c
> >index 252097f28b7634d13d91392b63abdf0640bcfa19..f4d48e89c49879ea282c9e7637e8255028e95758 100644
> >--- a/src/db/sysdb_ops.c
> >+++ b/src/db/sysdb_ops.c
> >@@ -159,6 +159,9 @@ int sss_ldb_modify_permissive(struct ldb_context *ldb,
> > 
> >     talloc_free(req);
> > 
> >+    if (ret) {
> >+        ret = sysdb_error_to_errno(ret);
> >+    }
> >     return ret;
> > }
> NACK
> 
> Current code expect LDB error codes to be returned from
> sss_ldb_modify_permissive. See src/db/sysdb_upgrade.c.
> But you can write doxygen comment for this function ;-)

D'oh. Thanks, I guess this is why we have code reviews.

OK, attached is a patch that adds comment to both the comment block
before the function and the return statement. I think adding the comment
is worth it because otherwise someone might spend an hour tracing why
does the code return ENOTDIR like I did before realizing it's actually
LDB_ERR_ATTRIBUTE_OR_VALUE_EXISTS :-)
-------------- next part --------------
>From a73b981e7b63a1d7a976bdc570184537de874769 Mon Sep 17 00:00:00 2001
From: Jakub Hrozek <jhrozek at redhat.com>
Date: Sat, 16 Aug 2014 19:41:40 +0200
Subject: [PATCH] SYSDB: Clarify sss_ldb_modify_permissive returns ldb error
 code

---
 src/db/sysdb_ops.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/src/db/sysdb_ops.c b/src/db/sysdb_ops.c
index 252097f28b7634d13d91392b63abdf0640bcfa19..e32d79a70c1c408f077b5a066acad9f200e36261 100644
--- a/src/db/sysdb_ops.c
+++ b/src/db/sysdb_ops.c
@@ -129,6 +129,9 @@ done:
  * The wrapper around ldb_modify that uses LDB_CONTROL_PERMISSIVE_MODIFY_OID
  * so that on adds entries that already exist are skipped and similarly
  * entries that are missing are ignored on deletes
+ *
+ * Please note this function returns LDB error codes, not sysdb error
+ * codes on purpose, see usage in callers!
  */
 int sss_ldb_modify_permissive(struct ldb_context *ldb,
                               struct ldb_message *msg)
@@ -159,6 +162,9 @@ int sss_ldb_modify_permissive(struct ldb_context *ldb,
 
     talloc_free(req);
 
+    /* Please note this function returns LDB error codes, not sysdb error
+     * codes on purpose, see usage in callers!
+     */
     return ret;
 }
 
-- 
1.9.3



More information about the sssd-devel mailing list