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

Pavel Březina pbrezina at redhat.com
Tue Nov 27 14:29:02 UTC 2012


On 11/27/2012 02:16 PM, Michal Židek wrote:
> On 11/27/2012 01:34 PM, Pavel Březina wrote:
>> 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.
>>
>>
>
> You are right. New patch attached.
> Thanks for the review Pavel.
>
> Michal

Ack!





More information about the sssd-devel mailing list