On 10/16/2014 04:58 PM, Sumit Bose wrote:
On Thu, Oct 16, 2014 at 04:39:12PM +0200, Pavel Březina wrote:
> On 10/16/2014 01:37 PM, Sumit Bose wrote:
>> On Wed, Oct 15, 2014 at 03:57:44PM +0200, Lukas Slebodnik wrote:
>>> On (15/10/14 15:37), Pavel Březina wrote:
>>>> On 10/14/2014 05:12 PM, Sumit Bose wrote:
>>>>> On Tue, Oct 14, 2014 at 11:24:04AM +0200, Sumit Bose wrote:
>>>>>> On Tue, Oct 14, 2014 at 11:16:28AM +0200, Sumit Bose wrote:
>>>>>>> On Wed, Oct 01, 2014 at 08:14:21PM +0200, Sumit Bose wrote:
>>>>>>>> On Mon, Sep 29, 2014 at 07:38:54PM +0200, Jakub Hrozek
wrote:
>>>>>>>>> On Mon, Sep 29, 2014 at 03:00:44PM +0200, Sumit Bose
wrote:
>>>>>>>>>>> Hi,
>>>>>>>>>>> I'm sending two more trivial debug
patches to squash in. Otherwise its an
>>>>>>>>>>> code wise ack. I'm running some tests
now.
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> Thank you. Jakub, in case there are no other
issues, can you add them
>>>>>>>>>> when committing the patches, or shall I send a
new round?
>>>>>>>>>>
>>>>>>>>>> bye,
>>>>>>>>>> Sumit
>>>>>>>>>
>>>>>>>>> I will run the patches through the IPA CI engine with
Tomas' help. If
>>>>>>>>> there are no regressions, I will squash in the
patches and push, if
>>>>>>>>> there are, we would do another round.
>>>>>>>>
>>>>>>>> Hi,
>>>>>>>>
>>>>>>>> this new series of patches contain fixes for various
issues found by
>>>>>>>> Pavel, Jakub and Alexander. There is an additional 9th
patch which
>>>>>>>> contains support for searching in the override values,
e.g. if the user
>>>>>>>> name is overridden we have to first search in the
override data and the
>>>>>>>> continue with the original object. See commit message for
details.
>>>>>>>
>>>>>>> Please find attached the latest version of the patches with
even more
>>>>>>> fixes for issue found by Alexander, Jakub and Pavel. I tired
to keep the
>>>>>>> original structure of the patches but a new patch sneaked in
which adds
>>>>>>> some new sysdb interface which allow to add attribute values
with
>>>>>>> detection of duplicates.
>>>>>>>
>>>>>>> bye,
>>>>>>> Sumit
>>>>>>
>>>>>> I forgot the say that the patches now depend on the two patches I
send
>>>>>> in the "[PATCHES] nss: add SSS_NSS_GETORIGBYNAME
request" thread.
>>>>>
>>>>> This new version fixes some minor issues found by Coverity.
>>>>
>>>> Hi,
>>
>> Pavel, Lukas, thank you for the review.
>>
>>>>
>>>> Patch 1: sysdb: add sysdb_update_view_name()
>>>>
>>>>> static errno_t sysdb_get_view_name_ex(TALLOC_CTX *mem_ctx,
>>>>> struct sysdb_ctx *sysdb,
>>>>> char **_view_name,
>>>>> bool *view_container_exists)
>>>>> {
>>>> ...
>>>>>
>>>>> *_view_name = talloc_strdup(mem_ctx, tmp_str);
>>>>
>>>> I think it is possible to just steal the value since ldb uses talloc. But
if
>>>> you want to strdup the string you should test for NULL.
>>
>> good point, I steal it.
>>
>>>>
>>>> Do we really need view_container_exists? I think it is not possible to
have a
>>>> view without a name and I would rather report an error in this case since
it
>>>> basically means that db is corrupted.
>>
>> I would prefer to keep it. You are right, currently the code always
>> creates the container with a name but I would like to be on the save
>> side and avoid issues when creating ing the container to a second time.
>>
>>>>
>>>>> ret = EOK;
>>>>>
>>>>> done:
>>>>> talloc_free(tmp_ctx);
>>>>> return ret;
>>>>> }
>>>>
>>>>> errno_t sysdb_update_view_name(struct sysdb_ctx *sysdb,
>>>>> const char *view_name)
>>>>> {
>>>>> errno_t ret;
>>>>> TALLOC_CTX *tmp_ctx;
>>>>> char *tmp_str;
>>>>> bool view_container_exists = false;
>>>>> bool add_view_name = false;
>>>>> struct ldb_message *msg;
>>>>
>>>> I suppose this function is not yet completed? add_view_name is basically
not
>>>> used since it is always true.
>>
>> yes, it will be used when we allow to change the view.
>>
>>>>
>>>>> if (ret == EOK) {
>>>>> if (strcmp(tmp_str, view_name) == 0) {
>>>>> /* view name already known, mothing to do */
>>>> ^ *typo*
>>
>> fixed
>>
>>>>
>>>>> DEBUG(SSSDBG_TRACE_ALL, "View name already in
place.\n");
>>>>> ret = EOK;
>>>>> goto done;
>>>>
>>>> Patch 2: Add sdap_deref_search_with_filter_send()
>>>>
>>>> ACK
>>>>
>>>> Patch 3: IPA: make IPA ID context available to extdom client code
>>>>
>>>> ACK
>>>>
>>>> Patch 4: IPA: add view support and get view name
>>>>
>>>> ACK
>>>>
>>>> Do you think the list of options is now complete? The new options are
still
>>>> missing in man page and config api.
>>
>> added
>>
>>>>
>>>> Patch 5: views: add ipa_get_ad_override_send()
>>>>
>>>> ACK
>>>>
>>>> Patch 6: sysdb: add sysdb_store_override
>>>>
>>>> ACK
>>>>
>>>> Patch 7: sysdb: sysdb_apply_default_override
>>>>
>>> Smal nitpick from our CI, after applying 7th path there is an error
>>>
>>> src/db/sysdb_views.c: In function 'safe_original_attributes':
>>> src/db/sysdb_views.c:539:13: error: implicit declaration of function
'sysdb_attrs_add_val_safe' [-Werror=implicit-function-declaration]
>>> ret = sysdb_attrs_add_val_safe(attrs, SYSDB_NAME_ALIAS,
>>> ^
>>> cc1: all warnings being treated as errors
>>>
>>> The solution is simple: just change order of patches.
>>> 0008-sysdb-add-sysdb_attrs_add_val_safe-and-sysdb_attrs_a.patch
>>> 0007-sysdb-sysdb_apply_default_override.patch
>>>
>>> The function sysdb_attrs_add_val_safe was defined in 8th patch,
>>> but was already used in 7th patch.
>>
>> The order is switched now.
>>
>> New version attached.
>>
>> bye,
>> Sumit
>
> Hi,
>
>> + <term>ipa_user_override_object_class
(string)</term>
>> + <listitem>
>> + <para>
>> + Name if the objectclass for user overrides. It
> ^ *of*
>> + is use to determine if the found override
object
> ^ *used*
>> + related to a user or a group.
> ^ *is related*
>> + </para>
>
> The same typos are also in description of ipa_group_override_object_class.
>
> Besides those typos it's a full ACK.
Thank you very much for the review. New version attached.
bye,
Sumit
ACK to all.