[SSSD] [PATCH] (2nd wave) Removing bad examples of usage of, sysdb_transaction_start/commit/end functions and making it, more consistent (all files except of src/db/sysdb_*.c)

Michal Židek mzidek at redhat.com
Thu Aug 23 10:48:41 UTC 2012


On 08/22/2012 04:48 PM, Jakub Hrozek wrote:
> On Tue, Aug 21, 2012 at 01:40:27PM +0200, Michal Židek wrote:
>> On 08/21/2012 10:39 AM, Pavel Březina wrote:
>>> On 08/20/2012 05:03 PM, Michal Židek wrote:
>>>> This is the second and final patch to make the usage of
>>>> sysdb_transaction_* functions
>>>> more consistent. There is still one place in the code left, where it is
>>>> not as it should be,
>>>> but it is related to a separate ticket (
>>>> https://fedorahosted.org/sssd/ticket/1370 ) and
>>>> should be done as a separate patch.
>>>>
>>>> The patch is attached.
>>>>
>>>> Thanks
>>>> Michal
>>> Nack.
>>>
>>> There are few places, where you use the original numeric debug levels.
>>> Please, replace those with SSSDBG macros.
>>> _______________________________________________
>>> sssd-devel mailing list
>>> sssd-devel at lists.fedorahosted.org
>>> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
>>
>> All new debug messages added in that patch are using SSSDBG macros.
>> I did not change already
>> existing messages, because it is not what I wanted to do in this
>> patch. But you are probably right, it
>> should be consistent. It is fixed in the new attached patch for
>> messages related to sysdb_transaction_* functions, but the rest
>> should be probably done as separate patch (if needed).
>>
>> Thanks
>> Michal
>> --- a/src/providers/ipa/ipa_selinux_common.c
>> +++ b/src/providers/ipa/ipa_selinux_common.c
>> @@ -32,12 +32,16 @@ errno_t ipa_save_user_maps(struct sysdb_ctx *sysdb,
>>                              struct sysdb_attrs **maps)
>>   {
>>       errno_t ret;
>> +    errno_t sret;
>> +    bool in_transaction = false;
>>       int i;
>>   
>>       ret = sysdb_transaction_start(sysdb);
>>       if (ret) {
>> +        DEBUG(SSSDBG_CRIT_FAILURE, ("Failed to start transaction\n"));
>>           goto done;
>>       }
>> +    in_transaction = true;
>>   
>>       for (i = 0; i < map_count; i++) {
>>           ret = sysdb_store_selinux_usermap(sysdb, maps[i]);
>> @@ -58,5 +62,11 @@ errno_t ipa_save_user_maps(struct sysdb_ctx *sysdb,
>>       ret = EOK;
>>   
>>   done:
>> +    if (in_transaction) {
>> +        sret = sysdb_transaction_cancel(sysdb);
>> +        if (sret != EOK) {
>> +            DEBUG(SSSDBG_CRIT_FAILURE, ("Failed to cancel transaction"));
>> +        }
>> +    }
>>       return ret;
>>   }
> You never set in_transaction to false in this  ^^^ function, please also
> add the boilerplate code to set in_transaction to false when the
> transaction is commited.
>
>> @@ -1973,16 +1986,19 @@ static errno_t sdap_nested_group_populate_users(TALLOC_CTX *mem_ctx,
>>   
>>       ret = sysdb_transaction_commit(sysdb);
>>       if (ret) {
>> -        DEBUG(1, ("Failed to commit transaction!\n"));
>> +        DEBUG(SSSDBG_CRIT_FAILURE, ("Failed to commit transaction!\n"));
>>           goto done;
>>       }
>> +    in_transaction = false;
>>   
>>       ret = EOK;
>>   done:
>>       if (ret != EOK) {
>> -        sret = sysdb_transaction_cancel(sysdb);
>> -        if (sret != EOK) {
>> -            DEBUG(2, ("Could not cancel transaction\n"));
>> +        if (in_transaction) {
>> +            sret = sysdb_transaction_cancel(sysdb);
>> +            if (sret != EOK) {
>> +                DEBUG(SSSDBG_CRIT_FAILURE, ("Could not cancel transaction\n"));
>> +            }
>>           }
>>           *_ghosts = NULL;
>>       } else {
> This is a nitpick but in general I think the transaction should be
> canceled just based on the value of in_transaction regardless of the
> value of "ret".
>
>> -done:
>>       ret = sysdb_transaction_commit(sysdb);
>>       if (ret != EOK) {
>> -        DEBUG(1, ("sysdb_transaction_commit failed.\n"));
>> -        goto fail;
>> +        DEBUG(SSSDBG_CRIT_FAILURE, ("sysdb_transaction_commit failed.\n"));
>> +        goto done;
>>       }
>>       in_transaction = false;
>>       ret = EOK;
>> -fail:
>> +
>> +done:
>>       if (in_transaction) {
>> -        sysdb_transaction_cancel(sysdb);
>> +        sret = sysdb_transaction_cancel(sysdb);
>> +        if (sret != EOK) {
>> +            DEBUG(SSSDBG_CRIT_FAILURE, ("Failed to cancel transaction\n"));
>> +        }
>>       }
>>       talloc_free(tmp_ctx);
>>       return ret;
> This merging of fail and done wouldn't work if all the groups were
> cached already.
>
> See the comment that says:
>      /* All groups are cached, nothing to do */
>
> Then the code jumps to done but is still within transaction which is
> then cancelled. It probably wouldn't matter because we don't change
> anything in the cache until then, but I think it would be nicer to start
> the transaction after the names were looked up. I don't think there's
> any risk of the db changing in the interim.
>
>> @@ -502,9 +503,10 @@ static void sdap_sudo_load_sudoers_done(struct tevent_req *subreq)
>>   
>>       /* commit transaction */
>>       ret = sysdb_transaction_commit(state->sysdb);
>> -    if (ret == EOK) {
>> -        in_transaction = false;
>> +    if (ret != EOK) {
>> +        DEBUG(SSSDBG_CRIT_FAILURE, ("Failed to commit transaction\n"));
> I think you forgot goto done here?
>
>>       }
>> +    in_transaction = false;
>>   
>>       DEBUG(SSSDBG_TRACE_FUNC, ("Sudoers is successfuly stored in cache\n"));
>>   
> The rest looks good to me.
> _______________________________________________
> sssd-devel mailing list
> sssd-devel at lists.fedorahosted.org
> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

New patch is attached.

Thanks
Michal

-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-2nd-wave-Removing-bad-examples-of-usage-of-sysdb_tra.patch
Type: text/x-patch
Size: 37825 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20120823/b51d0015/attachment.bin>


More information about the sssd-devel mailing list