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

Pavel Březina pbrezina at redhat.com
Tue Nov 27 12:34:41 UTC 2012


On 11/26/2012 03:18 PM, Michal Židek wrote:
> On 11/22/2012 07:16 PM, Pavel Březina wrote:
>> On 11/22/2012 06:47 PM, Jakub Hrozek wrote:
>>> On Thu, Nov 22, 2012 at 10:38:28AM +0100, Michal Židek wrote:
>>>> On 11/22/2012 10:27 AM, Pavel Březina wrote:
>>>>> On 11/21/2012 03:00 PM, Michal Židek wrote:
>>>>>> On 11/21/2012 11:04 AM, Jakub Hrozek wrote:
>>>>>>> On Tue, Nov 20, 2012 at 03:20:06PM +0100, Pavel Březina wrote:
>>>>>>>> We should propagate the built-in sid error instead of misusing id.
>>>>>>>> Maybe
>>>>>>>> return IDMAP* directly and return errno value in new output
>>>>>>>> parameter.
>>>>>>>>
>>>>>>>
>>>>>>> I actually think that using a special ID value is OK. We've been
>>>>>>> treating the UID and GID 0 as a special case before anyway for the
>>>>>>> fake
>>>>>>> users and groups. Also sdap_idmap_sid_to_unix() is supposed to
>>>>>>> return
>>>>>>> errno and not IDMAP* anyway, so even if we introduced a new IDMAP*
>>>>>>> return code, we would have to translate it into an (errno, id)
>>>>>>> tuple.
>>>>>>>
>>>>>>> The NSS responder would skip groups with a zero GID anyway.
>>>>>>>
>>>>>>
>>>>>> I let this as it was in the previous patch. The other things are
>>>>>> fixed.
>>>>>>
>>>>>> New patch attached.
>>>>>>
>>>>>> Thanks
>>>>>> Michal
>>>>>
>>>>> Nack.
>>>>>
>>>>>> +static bool sss_idmap_sid_is_builtin(const char *sid)
>>>>>> +{
>>>>>> +    if (strncmp(sid, "S-1-5-32-", 9) == 0) {
>>>>>> +        return true;
>>>>>> +    }
>>>>>> +
>>>>>> +    return true;
>>>>>
>>>>> should say false ^
>>>>>
>>>>> It looks good otherwise.
>>>>>
>>>>
>>>> New patch attached.
>>>>
>>>> Thanks
>>>> Michal
>>>>
>>>
>>> I was thinking about it some more and I think that Pavel is right, a
>>> special
>>> errno code would make the caller's code more readable. Maybe ENOTSUP
>>> would
>>> be usable?
>>
>> +1
>>
>>> Sorry I steered you in the wrong direction.
>>
>
> This patch does the same as previous, but uses the ENOTSUP to indicate
> that SID was processed.
>
> Thanks
> Michal

Hi,
two minor things.

>          ret = sdap_idmap_sid_to_unix(opts->idmap_ctx, sid_str, &gid);
> -        if (ret != EOK) {
> +        if (ret != EOK && ret != ENOTSUP) {
>              DEBUG(SSSDBG_MINOR_FAILURE,
>                    ("Could not convert SID string: [%s]\n",
>                     strerror(ret)));
>              goto fail;
> +        } else if (ret == ENOTSUP) {
> +            /* ENOTSUP is returned if built-in SID was provided
> +             * => fail to store the group, but return EOK */
> +            DEBUG(SSSDBG_TRACE_FUNC, ("Skipping built-in object.\n"));
> +            ret = EOK;
> +            goto fail;
>          }

Can you switch occurrences of similar code to:
if (ret == ENOTSUP) {
...
} else if (ret != EOK) {
...
}

> @@ -382,8 +388,10 @@ int sdap_save_user(TALLOC_CTX *memctx,
>      return EOK;
>
>  fail:
> -    DEBUG(2, ("Failed to save user [%s]\n",
> -              name ? name : "Unknown"));
> +    if (ret) {
> +        DEBUG(2, ("Failed to save user [%s]\n",
> +                  name ? name : "Unknown"));
> +    }
>      talloc_free(tmpctx);
>      return ret;
>  }

You should never get to fail label with ret == EOK. Can you switch to 
"done" pattern instead? I.e.

     talloc_steal(memctx, user_attrs);
     ret = EOK;

done:
     if (ret != EOK) {
         DEBUG(SSSDBG_OP_FAILURE, ("Failed to save user [%s]\n",
                                   name ? name : "Unknown"));
     }
     talloc_free(tmpctx);
     return ret;

Thanks,
Pavel.





More information about the sssd-devel mailing list