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

Michal Židek mzidek at redhat.com
Tue Nov 27 13:16:38 UTC 2012


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

-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-idmap-Silence-DEBUG-messages-when-dealing-with-built.patch
Type: text/x-patch
Size: 22361 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20121127/00a8bbbb/attachment.bin>


More information about the sssd-devel mailing list