[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)

Jakub Hrozek jhrozek at redhat.com
Thu Aug 23 16:12:27 UTC 2012


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 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
> 

Ack



More information about the sssd-devel mailing list