[SSSD] [PATCHES] Add backend part of user views

Pavel Březina pbrezina at redhat.com
Thu Oct 16 14:39:12 UTC 2014


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.




More information about the sssd-devel mailing list