On 09/05/2016 11:59 AM, Fabiano Fidêncio wrote:
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.
Hi Fabiano,
thanks for the code review. Please, see my inline comments below:
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.
I agree, it is better to us it this way. Addressed.
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.
I removed this definitions, it was useless.
But the reason is: The second argument of function ldb_ldif_write() is
pointer to function ldif_vprintf_fn. The condition on this is that
errors < 0, because ret >= is length of written debug message.
I wrote comment on it to the code. I am sorry, it wasn't obvious.
+ if (print_ctx == NULL) {
+ return -1;
+ return ERR_FN_ENOMEM;
+ }
I guess the return -1 is a leftover :-)
Right, it was leftover.
+ if (print_ctx->ldif == NULL) {
+ return -2;
+ return ERR_FN_ENOENT;
+ }
I guess the return -2 is also a leftover :-)
The same.
+ 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)
Addressed.
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,
Thanks :-), addressed.
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
There was the question about testing... every time SSSD writes to the
cache or ts_cache and debug_level in domain section is appropriate high,
ldif debug message appears.
debug_level = 0xFFFF0 in the domain section
sudo su -c "truncate -s0 /var/log/sssd/*.log"
systemctl restart sssd
sss_cache -E
getent passwd remote_user
You can try modify the user or delete him after. It will change the ldif
message.
PS: New patch set is attached.
Regards
--
Petr^4 Čech