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.