On Thu, Aug 23, 2012 at 12:48:41PM +0200, Michal Židek wrote:
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(a)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(a)lists.fedorahosted.org
>https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
New patch is attached.
Thanks
Michal
Ack