[PATCH] SYSDB: Fixing DB update

[Petr Cech]

Hi,

there is patch for #3023 attached.

Reason for this patch is explained in commit message.

I am open to discussion. In my opinion we don't need to connect sysdb 
pointer with domain structure because it is not used in other upgrade 
functions too.

Regards

[Lukas Slebodnik]

On (20/06/16 14:44), Petr Cech wrote:
> Hi,
>
> there is patch for #3023 attached.
>
> Reason for this patch is explained in commit message.
>
> I am open to discussion. In my opinion we don't need to connect sysdb pointer
> with domain structure because it is not used in other upgrade functions too.
>
> Regards
>
> -- 
> Petr^4 Čech
> From d0cee759147272f33427388af76ea66307e34881 Mon Sep 17 00:00:00 2001
> From: Petr Cech 
> Date: Mon, 20 Jun 2016 09:19:03 -0300
> Subject: [PATCH] SYSDB: Fixing DB update
>
> Functions sysdb_user_base_dn() and sysdb_group_base_dn() expect
> that struct sss_domain_info contains pointer to struct sysdb_ctx.
> This is not true in case of sysdb_upgrade functions.
> This patch fixes the situation and revert code to the state before
> 12a000c8c7c07259e438fb1e992134bdd07d9a30 commit.

I didn't test but it make sense.
It would be good to add comment to the code. So it will not be changed
in future (ldb_dn_new_fmt -> sysdb_user_*_dn)

Other alternative might be to "temporary initialize"
dom->sysdb and then set it back to NULL.
Or permanently initialize it sooner if sysdb is not destroyed
after upgrade.

Others might have different ideas/preferences as well.

LS

[Petr Cech]

On 06/22/2016 11:14 AM, Lukas Slebodnik wrote:
> On (20/06/16 14:44), Petr Cech wrote:
> > > Hi,
> > >
> > > there is patch for #3023 attached.
> > >
> > > Reason for this patch is explained in commit message.
> > >
> > > I am open to discussion. In my opinion we don't need to connect sysdb pointer
> > > with domain structure because it is not used in other upgrade functions too.
> > >
> > > Regards
> > >
> > > --
> > > Petr^4 Čech
> >
> > From d0cee759147272f33427388af76ea66307e34881 Mon Sep 17 00:00:00 2001
> > > From: Petr Cech 
> > > Date: Mon, 20 Jun 2016 09:19:03 -0300
> > > Subject: [PATCH] SYSDB: Fixing DB update
> > >
> > > Functions sysdb_user_base_dn() and sysdb_group_base_dn() expect
> > > that struct sss_domain_info contains pointer to struct sysdb_ctx.
> > > This is not true in case of sysdb_upgrade functions.
> > > This patch fixes the situation and revert code to the state before
> > > 12a000c8c7c07259e438fb1e992134bdd07d9a30 commit.
>
> I didn't test but it make sense.
> It would be good to add comment to the code. So it will not be changed
> in future (ldb_dn_new_fmt -> sysdb_user_*_dn)
>
> Other alternative might be to "temporary initialize"
> dom->sysdb and then set it back to NULL.
> Or permanently initialize it sooner if sysdb is not destroyed
> after upgrade.
>
> Others might have different ideas/preferences as well.
>
> LS

Hi Lukas,

thanks for comment. I agree that we should protect this part of code 
from the changes in future.

I vote for comment in code. So, there is new patch attached.

Regards

[Petr Cech]

bump :-)

[Sumit Bose]

On Wed, Jun 22, 2016 at 03:30:28PM +0200, Petr Cech wrote:
> On 06/22/2016 11:14 AM, Lukas Slebodnik wrote:
> > On (20/06/16 14:44), Petr Cech wrote:
> > > > Hi,
> > > >
> > > > there is patch for #3023 attached.
> > > >
> > > > Reason for this patch is explained in commit message.
> > > >
> > > > I am open to discussion. In my opinion we don't need to connect sysdb pointer
> > > > with domain structure because it is not used in other upgrade functions too.
> > > >
> > > > Regards
> > > >
> > > > --
> > > > Petr^4 Čech
> > >
> > > From d0cee759147272f33427388af76ea66307e34881 Mon Sep 17 00:00:00 2001
> > > > From: Petr Cech 
> > > > Date: Mon, 20 Jun 2016 09:19:03 -0300
> > > > Subject: [PATCH] SYSDB: Fixing DB update
> > > >
> > > > Functions sysdb_user_base_dn() and sysdb_group_base_dn() expect
> > > > that struct sss_domain_info contains pointer to struct sysdb_ctx.
> > > > This is not true in case of sysdb_upgrade functions.
> > > > This patch fixes the situation and revert code to the state before
> > > > 12a000c8c7c07259e438fb1e992134bdd07d9a30 commit.
> >
> > I didn't test but it make sense.
> > It would be good to add comment to the code. So it will not be changed
> > in future (ldb_dn_new_fmt -> sysdb_user_*_dn)
> >
> > Other alternative might be to "temporary initialize"
> > dom->sysdb and then set it back to NULL.
> > Or permanently initialize it sooner if sysdb is not destroyed
> > after upgrade.
> >
> > Others might have different ideas/preferences as well.
> >
> > LS
>
> Hi Lukas,
>
> thanks for comment. I agree that we should protect this part of code from
> the changes in future.
>
> I vote for comment in code. So, there is new patch attached.

Thank you, I think comments are ok for the update code.

I tested the version 10 update by just setting the version in the cache
file to 0.10 and it fails without the patch and works as expected with
the patch (please note that later on another update fails becasue we add
an index item unconditionally without checking if it already exists and
I started with a 0.17 cache).

There is just a minor typo (see below) which might also be fixed while
pushing the patch.

Just waiting for CI ...

bye,
Sumit
> Regards
>
> -- 
> Petr^4 Čech
> From 729e8bb6b5c03d14b423c5dcccd46ae1cb4ebcdc Mon Sep 17 00:00:00 2001
> From: Petr Cech 
> Date: Mon, 20 Jun 2016 09:19:03 -0300
> Subject: [PATCH] SYSDB: Fixing DB update
>
> Functions sysdb_user_base_dn() and sysdb_group_base_dn() expect
> that struct sss_domain_info contains pointer to struct sysdb_ctx.
> This is not true in case of sysdb_upgrade functions.
> This patch fixes the situation and revert code to the state before
> 12a000c8c7c07259e438fb1e992134bdd07d9a30 commit.
>
> Resolves:
> https://fedorahosted.org/sssd/ticket/3023
>
> src/db/sysdb_upgrade.c | 22 +++++++++++++++++++---
>  1 file changed, 19 insertions(+), 3 deletions(-)
>
> diff --git a/src/db/sysdb_upgrade.c b/src/db/sysdb_upgrade.c
> index 113f24644e3e3de1d4c46d375492c2fe1e6b2f83..f68800eca6b362b62dbd30eecfa607d4b577df22 100644
> --- a/src/db/sysdb_upgrade.c
> +++ b/src/db/sysdb_upgrade.c
> @@ -443,12 +443,23 @@ int sysdb_check_upgrade_02(struct sss_domain_info *domains,
>              goto done;
>          }
>    users_dn = sysdb_user_base_dn(tmp_ctx, dom);
>    /*
>     * dom->sysdb->ldb is not initialized,
>     * so ldb_dn_new_fmt() shouldn't be changed to sysdb_*_base_dn()
>     */
>    users_dn = ldb_dn_new_fmt(tmp_ctx, sysdb->ldb,
>                             SYSDB_TMPL_USER_BASE, dom->name);

^ missing space
>      if (!users_dn) {
>          ret = ENOMEM;
>          goto done;
>      }
>    groups_dn = sysdb_group_base_dn(tmp_ctx, dom);
>    /*
>     * dom->sysdb->ldb is not initialized,
>     * so ldb_dn_new_fmt() shouldn't be changed to sysdb_*_base_dn()
>     */
>    groups_dn = ldb_dn_new_fmt(tmp_ctx, sysdb->ldb,
>                               SYSDB_TMPL_GROUP_BASE, dom->name);
>    if (!groups_dn) {
>        ret = ENOMEM;
>        goto done;
>
> @@ -1045,7 +1056,12 @@ int sysdb_upgrade_10(struct sysdb_ctx *sysdb, struct sss_domain_info *domain,
>          return ret;
>      }
> basedn = sysdb_user_base_dn(tmp_ctx, domain);
> /*
> * dom->sysdb->ldb is not initialized,
> * so ldb_dn_new_fmt() shouldn't be changed to sysdb_*_base_dn()
> */
> basedn = ldb_dn_new_fmt(tmp_ctx, sysdb->ldb,
>                        SYSDB_TMPL_USER_BASE, domain->name);
> if (basedn == NULL) {
>        ret = EIO;
>        goto done;
>
> -- 
> 2.5.5
> sssd-devel mailing list
> [e-mail]
> https://lists.fedorahosted.org/admin/lists/[e-mail]

[Sumit Bose]

On Mon, Jul 11, 2016 at 10:29:56AM +0200, Sumit Bose wrote:
> On Wed, Jun 22, 2016 at 03:30:28PM +0200, Petr Cech wrote:
> > On 06/22/2016 11:14 AM, Lukas Slebodnik wrote:
> > > On (20/06/16 14:44), Petr Cech wrote:
> > > > > Hi,
> > > > >
> > > > > there is patch for #3023 attached.
> > > > >
> > > > > Reason for this patch is explained in commit message.
> > > > >
> > > > > I am open to discussion. In my opinion we don't need to connect sysdb pointer
> > > > > with domain structure because it is not used in other upgrade functions too.
> > > > >
> > > > > Regards
> > > > >
> > > > > --
> > > > > Petr^4 Čech
> > > >
> > > > From d0cee759147272f33427388af76ea66307e34881 Mon Sep 17 00:00:00 2001
> > > > > From: Petr Cech 
> > > > > Date: Mon, 20 Jun 2016 09:19:03 -0300
> > > > > Subject: [PATCH] SYSDB: Fixing DB update
> > > > >
> > > > > Functions sysdb_user_base_dn() and sysdb_group_base_dn() expect
> > > > > that struct sss_domain_info contains pointer to struct sysdb_ctx.
> > > > > This is not true in case of sysdb_upgrade functions.
> > > > > This patch fixes the situation and revert code to the state before
> > > > > 12a000c8c7c07259e438fb1e992134bdd07d9a30 commit.
> > >
> > > I didn't test but it make sense.
> > > It would be good to add comment to the code. So it will not be changed
> > > in future (ldb_dn_new_fmt -> sysdb_user_*_dn)
> > >
> > > Other alternative might be to "temporary initialize"
> > > dom->sysdb and then set it back to NULL.
> > > Or permanently initialize it sooner if sysdb is not destroyed
> > > after upgrade.
> > >
> > > Others might have different ideas/preferences as well.
> > >
> > > LS
> >
> > Hi Lukas,
> >
> > thanks for comment. I agree that we should protect this part of code from
> > the changes in future.
> >
> > I vote for comment in code. So, there is new patch attached.
>
> Thank you, I think comments are ok for the update code.
>
> I tested the version 10 update by just setting the version in the cache
> file to 0.10 and it fails without the patch and works as expected with
> the patch (please note that later on another update fails becasue we add
> an index item unconditionally without checking if it already exists and
> I started with a 0.17 cache).
>
> There is just a minor typo (see below) which might also be fixed while
> pushing the patch.
>
> Just waiting for CI ...

... passed http://sssd-ci.duckdns.org/logs/job/49/45/summary.html

ACK

bye,
Sumit
> bye,
> Sumit
> > Regards
> >
> > -- 
> > Petr^4 Čech
> > From 729e8bb6b5c03d14b423c5dcccd46ae1cb4ebcdc Mon Sep 17 00:00:00 2001
> > From: Petr Cech 
> > Date: Mon, 20 Jun 2016 09:19:03 -0300
> > Subject: [PATCH] SYSDB: Fixing DB update
> >
> > Functions sysdb_user_base_dn() and sysdb_group_base_dn() expect
> > that struct sss_domain_info contains pointer to struct sysdb_ctx.
> > This is not true in case of sysdb_upgrade functions.
> > This patch fixes the situation and revert code to the state before
> > 12a000c8c7c07259e438fb1e992134bdd07d9a30 commit.
> >
> > Resolves:
> > https://fedorahosted.org/sssd/ticket/3023
> >
> > src/db/sysdb_upgrade.c | 22 +++++++++++++++++++---
> >  1 file changed, 19 insertions(+), 3 deletions(-)
> >
> > diff --git a/src/db/sysdb_upgrade.c b/src/db/sysdb_upgrade.c
> > index 113f24644e3e3de1d4c46d375492c2fe1e6b2f83..f68800eca6b362b62dbd30eecfa607d4b577df22 100644
> > --- a/src/db/sysdb_upgrade.c
> > +++ b/src/db/sysdb_upgrade.c
> > @@ -443,12 +443,23 @@ int sysdb_check_upgrade_02(struct sss_domain_info *domains,
> >              goto done;
> >          }
> >    users_dn = sysdb_user_base_dn(tmp_ctx, dom);
> >    /*
> >     * dom->sysdb->ldb is not initialized,
> >     * so ldb_dn_new_fmt() shouldn't be changed to sysdb_*_base_dn()
> >     */
> >    users_dn = ldb_dn_new_fmt(tmp_ctx, sysdb->ldb,
> >                             SYSDB_TMPL_USER_BASE, dom->name);
>                                 ^ missing space
>
> >      if (!users_dn) {
> >          ret = ENOMEM;
> >          goto done;
> >      }
> >    groups_dn = sysdb_group_base_dn(tmp_ctx, dom);
> >    /*
> >     * dom->sysdb->ldb is not initialized,
> >     * so ldb_dn_new_fmt() shouldn't be changed to sysdb_*_base_dn()
> >     */
> >    groups_dn = ldb_dn_new_fmt(tmp_ctx, sysdb->ldb,
> >                               SYSDB_TMPL_GROUP_BASE, dom->name);
> >    if (!groups_dn) {
> >        ret = ENOMEM;
> >        goto done;
> >
> > @@ -1045,7 +1056,12 @@ int sysdb_upgrade_10(struct sysdb_ctx *sysdb, struct sss_domain_info *domain,
> >          return ret;
> >      }
> > basedn = sysdb_user_base_dn(tmp_ctx, domain);
> > /*
> > * dom->sysdb->ldb is not initialized,
> > * so ldb_dn_new_fmt() shouldn't be changed to sysdb_*_base_dn()
> > */
> > basedn = ldb_dn_new_fmt(tmp_ctx, sysdb->ldb,
> >                        SYSDB_TMPL_USER_BASE, domain->name);
> > if (basedn == NULL) {
> >        ret = EIO;
> >        goto done;
> >
> > -- 
> > 2.5.5
> > sssd-devel mailing list
> > [e-mail]
> > https://lists.fedorahosted.org/admin/lists/[e-mail]
>
> sssd-devel mailing list
> [e-mail]
> https://lists.fedorahosted.org/admin/lists/[e-mail]

[Lukas Slebodnik]

On (11/07/16 10:55), Sumit Bose wrote:
> On Mon, Jul 11, 2016 at 10:29:56AM +0200, Sumit Bose wrote:
> > On Wed, Jun 22, 2016 at 03:30:28PM +0200, Petr Cech wrote:
> > > On 06/22/2016 11:14 AM, Lukas Slebodnik wrote:
> > > > On (20/06/16 14:44), Petr Cech wrote:
> > > > > > Hi,
> > > > > >
> > > > > > there is patch for #3023 attached.
> > > > > >
> > > > > > Reason for this patch is explained in commit message.
> > > > > >
> > > > > > I am open to discussion. In my opinion we don't need to connect sysdb pointer
> > > > > > with domain structure because it is not used in other upgrade functions too.
> > > > > >
> > > > > > Regards
> > > > > >
> > > > > > --
> > > > > > Petr^4 Čech
> > > > >
> > > > > From d0cee759147272f33427388af76ea66307e34881 Mon Sep 17 00:00:00 2001
> > > > > > From: Petr Cech 
> > > > > > Date: Mon, 20 Jun 2016 09:19:03 -0300
> > > > > > Subject: [PATCH] SYSDB: Fixing DB update
> > > > > >
> > > > > > Functions sysdb_user_base_dn() and sysdb_group_base_dn() expect
> > > > > > that struct sss_domain_info contains pointer to struct sysdb_ctx.
> > > > > > This is not true in case of sysdb_upgrade functions.
> > > > > > This patch fixes the situation and revert code to the state before
> > > > > > 12a000c8c7c07259e438fb1e992134bdd07d9a30 commit.
> > > >
> > > > I didn't test but it make sense.
> > > > It would be good to add comment to the code. So it will not be changed
> > > > in future (ldb_dn_new_fmt -> sysdb_user_*_dn)
> > > >
> > > > Other alternative might be to "temporary initialize"
> > > > dom->sysdb and then set it back to NULL.
> > > > Or permanently initialize it sooner if sysdb is not destroyed
> > > > after upgrade.
> > > >
> > > > Others might have different ideas/preferences as well.
> > > >
> > > > LS
> > >
> > > Hi Lukas,
> > >
> > > thanks for comment. I agree that we should protect this part of code from
> > > the changes in future.
> > >
> > > I vote for comment in code. So, there is new patch attached.
> >
> > Thank you, I think comments are ok for the update code.
> >
> > I tested the version 10 update by just setting the version in the cache
> > file to 0.10 and it fails without the patch and works as expected with
> > the patch (please note that later on another update fails becasue we add
> > an index item unconditionally without checking if it already exists and
> > I started with a 0.17 cache).
> >
> > There is just a minor typo (see below) which might also be fixed while
> > pushing the patch.
> >
> > Just waiting for CI ...
>
> ... passed http://sssd-ci.duckdns.org/logs/job/49/45/summary.html
>
> ACK

master:
* 311836214245600566f881ff6253594e0999008e

sssd-1-13:
* 0a7784d3584a5a6db1e0251e1bcc9dd1790f1f38

LS

[Petr Cech]

On 07/11/2016 11:59 AM, Lukas Slebodnik wrote:
> On (11/07/16 10:55), Sumit Bose wrote:
> > On Mon, Jul 11, 2016 at 10:29:56AM +0200, Sumit Bose wrote:
> > > On Wed, Jun 22, 2016 at 03:30:28PM +0200, Petr Cech wrote:
> > > > On 06/22/2016 11:14 AM, Lukas Slebodnik wrote:
> > > > > On (20/06/16 14:44), Petr Cech wrote:
> > > > > > > Hi,
> > > > > > >
> > > > > > > there is patch for #3023 attached.
> > > > > > >
> > > > > > > Reason for this patch is explained in commit message.
> > > > > > >
> > > > > > > I am open to discussion. In my opinion we don't need to connect sysdb pointer
> > > > > > > with domain structure because it is not used in other upgrade functions too.
> > > > > > >
> > > > > > > Regards
> > > > > > >
> > > > > > > --
> > > > > > > Petr^4 Čech
> > > > > > From d0cee759147272f33427388af76ea66307e34881 Mon Sep 17 00:00:00 2001
> > > > > > > From: Petr Cech 
> > > > > > > Date: Mon, 20 Jun 2016 09:19:03 -0300
> > > > > > > Subject: [PATCH] SYSDB: Fixing DB update
> > > > > > >
> > > > > > > Functions sysdb_user_base_dn() and sysdb_group_base_dn() expect
> > > > > > > that struct sss_domain_info contains pointer to struct sysdb_ctx.
> > > > > > > This is not true in case of sysdb_upgrade functions.
> > > > > > > This patch fixes the situation and revert code to the state before
> > > > > > > 12a000c8c7c07259e438fb1e992134bdd07d9a30 commit.
> > > > > > >
> > > > >
> > > > > I didn't test but it make sense.
> > > > > It would be good to add comment to the code. So it will not be changed
> > > > > in future (ldb_dn_new_fmt -> sysdb_user_*_dn)
> > > > >
> > > > > Other alternative might be to "temporary initialize"
> > > > > dom->sysdb and then set it back to NULL.
> > > > > Or permanently initialize it sooner if sysdb is not destroyed
> > > > > after upgrade.
> > > > >
> > > > > Others might have different ideas/preferences as well.
> > > > >
> > > > > LS
> > > >
> > > > Hi Lukas,
> > > >
> > > > thanks for comment. I agree that we should protect this part of code from
> > > > the changes in future.
> > > >
> > > > I vote for comment in code. So, there is new patch attached.
> > >
> > > Thank you, I think comments are ok for the update code.
> > >
> > > I tested the version 10 update by just setting the version in the cache
> > > file to 0.10 and it fails without the patch and works as expected with
> > > the patch (please note that later on another update fails becasue we add
> > > an index item unconditionally without checking if it already exists and
> > > I started with a 0.17 cache).
> > >
> > > There is just a minor typo (see below) which might also be fixed while
> > > pushing the patch.
> > >
> > > Just waiting for CI ...
> >
> > ... passed http://sssd-ci.duckdns.org/logs/job/49/45/summary.html
> >
> > ACK
>
> master:
> 311836214245600566f881ff6253594e0999008e
>
> sssd-1-13:
> 0a7784d3584a5a6db1e0251e1bcc9dd1790f1f38
>
> LS

Sumit, thanks for code review.
Lukas, thanks for typo fixing :-)