Petr,
I went through your patches and in general they look good to me.
However, I haven't done any tests yet with your patches (and I'll do
it after lunch).
Please, below you can see a few comments. Feel completely free to
ignore the first one if you feel like doing it, it's just a minor :-)
For the other comments, I'd like to understand a few changes you have done.
Patch 0001: SYSDB: Adding message to inform which cache is used
About the following part of the patch:
+static const char *get_attr_storage(int state_mask)
+{
+ const char *storage = "";
+
+ if (state_mask == SSS_SYSDB_BOTH_CACHE ) {
+ storage = "cache, ts_cache";
+ } else if (state_mask == SSS_SYSDB_TS_CACHE) {
+ storage = "ts_cache";
+ } else if (state_mask == SSS_SYSDB_CACHE) {
+ storage = "cache";
+ }
+
+ return storage;
+}
I personally don't like this kind of comparison done with flags. I'd
go for something like: if ((state_mask & SSS_SYSDB_BOTH_CACHE) != 0)
...
But this is a really minor and feel free to ignore it.
Patch 0002: SYSDB: Adding message about reason why cache changed
LGTM
Patch 0003: SYSDB: Adding wrappers for ldb_* operations
About the following parts of the patch:
On src/db/sysdb_ldb_wrapper.c
+#define ERR_FN_ENOMEM (-1 * ENOMEM)
+#define ERR_FN_ENOENT (-1 * ENOENT)
Why? I failed to understand why you're doing this here.
+ if (print_ctx == NULL) {
+ return -1;
+ return ERR_FN_ENOMEM;
+ }
I guess the return -1 is a leftover :-)
+ if (print_ctx->ldif == NULL) {
+ return -2;
+ return ERR_FN_ENOENT;
+ }
I guess the return -2 is also a leftover :-)
+ if (ret < 0) {
+ DEBUG(SSSDBG_MINOR_FAILURE, "ldb_ldif_write() failed with [%d][%s].\n",
+ -1 * ret, sss_strerror(-1 * ret));
+ goto done;
+ }
And here again this dance multiplying by -1 that I don't understand
the reason :-\
+done:
+ if (ldb_print_ctx != NULL && ldb_print_ctx->ldif != NULL) {
+ talloc_free(ldb_print_ctx->ldif);
+ }
+ talloc_free(ldb_print_ctx);
AFAIU talloc_free can gracefully handle NULL. Considering that's the
case I'd just check for (if ldb_print_ctx != NULL)
talloc_free(ldb_print_ctx->ldif);
Considering it doesn't, we may have some issues on trying to free
(ldb_print_ctx)
On src/db/sysdb_ldb_wrapper.h:
+int sss_ldb_rename(struct ldb_context *ldb,
+ struct ldb_dn * olddn,
+ struct ldb_dn *newdn);
Just a really minor codying style change here, remove the extra space
between * and olddn: struct ldb_dn * olddn, -> struct ldb_dn *olddn,
Patch0004: SYSDB: ldb_add --> sss_ldb_add in sysdb
Patch0005: SYSDB: ldb_delete --> sss_ldb_delete in sysdb
Patch0006: SYSDB: ldb_modify --> sss_ldb_modify in sysdb
Patch0007: SYSDB: ldb_rename --> sss_ldb_rename in sysdb
LGTM
Best Regards,
--
Fabiano Fidêncio
_______________________________________________
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
