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

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

[Pavel Březina]

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.

[Michal Židek]

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
> [e-mail]
> 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

[Pavel Březina]

On 08/21/2012 01:40 PM, 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
> > [e-mail]
> > 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

Ack.

[Jakub Hrozek]

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
> > [e-mail]
> > 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.

[Michal Židek]

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
> > > [e-mail]
> > > 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
> [e-mail]
> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

New patch is attached.

Thanks
Michal

[Jakub Hrozek]

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
> > > > [e-mail]
> > > > 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
> > [e-mail]
> > https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
>
> New patch is attached.
>
> Thanks
> Michal

Ack

[Jakub Hrozek]

On Thu, Aug 23, 2012 at 06:12:27PM +0200, Jakub Hrozek wrote:
> > New patch is attached.
> >
> > Thanks
> > Michal
>
> Ack

I amended the commit message (it was too long) and pushed the patch to
master.