On 08/30/2016 01:31 PM, Pavel Březina wrote:
On 08/30/2016 01:29 PM, Pavel Březina wrote:
On 08/04/2016 08:28 AM, Fabiano Fidêncio wrote:
On Thu, Aug 4, 2016 at 8:08 AM, Petr Cech <pc...@redhat.com> wrote:
On 08/03/2016 09:54 AM, Fabiano Fidêncio wrote:
Hey!
I'd do it a bit differently.
Hello Fabiano,
I am glad for another point of view.
diff --git a/src/providers/ldap/sdap_async_users.c
b/src/providers/ldap/sdap_async_users.c
index
e44c045b3f8ff6aed33a42cf2919bc01aa41a243..3a8efa4caacbad74f493de334a387104d0e7cec4
100644
--- a/src/providers/ldap/sdap_async_users.c
+++ b/src/providers/ldap/sdap_async_users.c
@@ -519,6 +519,7 @@ int sdap_save_users(TALLOC_CTX *memctx,
char **_usn_value)
{
TALLOC_CTX *tmpctx;
+ const char* user_name = NULL;
^-----
I know this should be 'const char *'. I will fix it.
char *higher_usn = NULL;
char *usn_value;
int ret;
@@ -548,14 +549,22 @@ int sdap_save_users(TALLOC_CTX *memctx,
for (i = 0; i < num_users; i++) {
usn_value = NULL;
+ ret = sdap_get_user_primary_name(memctx, opts, users[i],
+ dom, &user_name);
+ if (ret != EOK) {
+ DEBUG(SSSDBG_OP_FAILURE, "Failed to get user name\n");
+ goto done;
+ }
+
IMO, if it fails, that's okay, then let's just go for using the loop
index.
IOW, please, remove the "goto done;" line.
Right, I will fix it.
ret = sdap_save_user(tmpctx, opts, dom, users[i],
&usn_value,
now);
/* Do not fail completely on errors.
* Just report the failure to save and go on */
if (ret) {
- DEBUG(SSSDBG_OP_FAILURE, "Failed to store user %d.
Ignoring.\n", i);
+ DEBUG(SSSDBG_OP_FAILURE, "Failed to store user %s.
Ignoring.\n",
+ user_name);
} else {
- DEBUG(SSSDBG_TRACE_ALL, "User %d processed!\n", i);
+ DEBUG(SSSDBG_TRACE_ALL, "User %s processed!\n",
user_name);
}
I didn't detailed check what sdap_get_user_primary_name() is doing,
but I have the feeling that when it fails, user_name is NULL.
So, when printing the debug message, you can check whether the
username is not NULL and print it, otherwise you could print the
array's index as it was done before your patch.
I did some investigation.
If error occurs in function sdap_get_user_primary_name(), the function
doesn't touch return argument (here it is &user_name), so there
needn't be a
NULL value in user_name.
I discussed this topic with Lukas offline. Our practise is:
int function(input_arg, ..., _output_arg)
if (ret != 0) than there is no guarantee that _output_arg makes sense.
In our case, _output_arg isn't NULL. It's a pitty.
Why not? :-)
You set user_name to NULL in the beginning of the function and it is
just set again when sdap_get_primary_fqdn() is successful.
Checking whether _output_arg is NULL or not seems sane in this case.
There are some comments about using functions such
sdap_get_user_primary_name() in thread 'LDAP: Do not print "null" in
the
DEBUG message' (I need read this, thanks Lukas for info).
I'll check it out as well :-)
If this function fails, we may as well quit processing this user and
continue with the next one, since if it doesn't have a name, it is an
invalid record and we can't store it (it fails in subsequent call of
sdap_save_user).
BTW This is what we usually do when we can't get a name for debug
message in a cycle.
Hello,
Pavel, thanks for reminder, I solve similar case for groups now.
Please, see attached patch.
I removed DEBUG if sdap_get_user_primary_name() failes because it
is not important from global point of view. And there is no goto :-)
I hope this little improvement will be helpful for all admins.
Regards
--
Petr^4 Čech
>From b3ae463a7544bb9561126c5e05475d5b98928edc Mon Sep 17 00:00:00 2001
From: Petr Cech <pc...@redhat.com>
Date: Fri, 15 Jul 2016 14:54:35 +0200
Subject: [PATCH] LDAP: Improving debug message
There were debug messges refering to user by for loop variable.
We might obtain user name and refer to user by this.
---
src/providers/ldap/sdap_async_users.c | 21 +++++++++++++++++++--
1 file changed, 19 insertions(+), 2 deletions(-)
diff --git a/src/providers/ldap/sdap_async_users.c b/src/providers/ldap/sdap_async_users.c
index 87d91d8247c37a4c6a1d83b7189399056528fc90..17d8680d82b25946e975034b852e24fcb731b535 100644
--- a/src/providers/ldap/sdap_async_users.c
+++ b/src/providers/ldap/sdap_async_users.c
@@ -540,6 +540,7 @@ int sdap_save_users(TALLOC_CTX *memctx,
char **_usn_value)
{
TALLOC_CTX *tmpctx;
+ const char *user_name = NULL;
char *higher_usn = NULL;
char *usn_value;
int ret;
@@ -569,14 +570,30 @@ int sdap_save_users(TALLOC_CTX *memctx,
for (i = 0; i < num_users; i++) {
usn_value = NULL;
+ ret = sdap_get_user_primary_name(memctx, opts, users[i],
+ dom, &user_name);
+ if (ret != EOK) {
+ user_name = NULL;
+ }
+
ret = sdap_save_user(tmpctx, opts, dom, users[i], &usn_value, now);
/* Do not fail completely on errors.
* Just report the failure to save and go on */
if (ret) {
- DEBUG(SSSDBG_OP_FAILURE, "Failed to store user %d. Ignoring.\n", i);
+ if (user_name != NULL) {
+ DEBUG(SSSDBG_OP_FAILURE, "Failed to store user %s. Ignoring.\n",
+ user_name);
+ } else {
+ DEBUG(SSSDBG_OP_FAILURE, "Failed to store user %d. Ignoring.\n",
+ i);
+ }
} else {
- DEBUG(SSSDBG_TRACE_ALL, "User %d processed!\n", i);
+ if (user_name != NULL) {
+ DEBUG(SSSDBG_TRACE_ALL, "User %s processed!\n", user_name);
+ } else {
+ DEBUG(SSSDBG_TRACE_ALL, "User %d processed!\n", i);
+ }
}
if (usn_value) {
--
2.7.4
_______________________________________________
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
