[SSSD] [PATCH] idmap: Silence DEBUG messages when dealing with built-in, SIDs.

Pavel Březina pbrezina at redhat.com
Tue Nov 20 14:20:06 UTC 2012


On 11/15/2012 09:11 PM, Michal Židek wrote:
> https://fedorahosted.org/sssd/ticket/1593
>
> With this patch we get:
>
> [sssd[be[AD]]] [sdap_save_group] (0x1000): Mapping group
> [Administrators] objectSID to unix ID
> [sssd[be[AD]]] [sdap_idmap_sid_to_unix] (0x0400): Object SID
> [S-1-5-32-544] is a built-in one.
> [sssd[be[AD]]] [sdap_save_group] (0x0400): Skipping built-in object.
>
> Instead of the old messages, that looked like this:
>
> [sssd[be[AD]]] [sdap_save_group] (0x1000): Mapping group
> [Administrators] objectSID to unix ID
> [sssd[be[AD]]] [sdap_idmap_sid_to_unix] (0x0080): Could not parse domain
> SID from [S-1-5-32-544]
> [sssd[be[AD]]] [sdap_save_group] (0x0080): Could not convert SID string:
> [Invalid argument]
> [sssd[be[AD]]] [sdap_save_group] (0x0080): Failed to save group
> [Administrators]: [Invalid argument]
> [sssd[be[AD]]] [sdap_save_groups] (0x0040): Failed to store group 0.
> Ignoring.
>
>
> The patch is attached.
>
> NOTE: To reproduce, set enumerate = true for the AD domain, delete the
> cache files and start SSSD.
>
> Thanks
> Michal

Nack.

> +static bool sss_idmap_sid_is_builtin(const char *sid)
> +{
> +    if (strncmp(sid, "S-1-5-32-", 9)) {
> +        return false;
> +    } else {
> +        return true;
> +    }
> +}

Please, change ^^ to:

+static bool sss_idmap_sid_is_builtin(const char *sid)
+{
+    if (strncmp(sid, "S-1-5-32-", 9) == 0) {
+        return true;
+    }
+
+    return false;
+}

> @@ -380,7 +380,8 @@ sdap_idmap_sid_to_unix(struct sdap_idmap_ctx *idmap_ctx,
>      err = sss_idmap_sid_to_unix(idmap_ctx->map,
>                                  sid_str,
>                                  (uint32_t *)id);
> -    if (err != IDMAP_SUCCESS && err != IDMAP_NO_DOMAIN) {
> +    if (err != IDMAP_SUCCESS && err != IDMAP_NO_DOMAIN &&
> +        err != IDMAP_BUILTIN_SID) {
>          DEBUG(SSSDBG_MINOR_FAILURE,
>                ("Could not convert objectSID [%s] to a UNIX ID\n",
>                	sid_str));
> @@ -420,6 +421,10 @@ sdap_idmap_sid_to_unix(struct sdap_idmap_ctx *idmap_ctx,
>              ret = EIO;
>              goto done;
>          }
> +    } else if (err == IDMAP_BUILTIN_SID) {

Can you use switch statement please? Like:

switch (err) {
     case IDMAP_NO_DOMAIN:
         ...
         break;
     case IDMAP_BUILTIN_SID:
         ...
         break;
     default:
         error
}

> +        DEBUG(SSSDBG_TRACE_FUNC,
> +              ("Object SID [%s] is a built-in one.\n", sid_str));
> +        *id = 0; /* O indicates, that this ID should be ignored */
>

We should propagate the built-in sid error instead of misusing id. Maybe
return IDMAP* directly and return errno value in new output parameter.

Thanks,
Pavel.




More information about the sssd-devel mailing list