[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