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.