[PATCH] AD: inicialize root_domain_attrs field

[Pavel Reichl]

Hello,

please see simple patch attached.

Thanks!

[Jakub Hrozek]

On Thu, Sep 24, 2015 at 05:05:20PM +0200, Pavel Reichl wrote:
> Hello,
>
> please see simple patch attached.
>
> Thanks!

Thanks, this would solve the bug, but can you also change the allocation
of the structure from:
    req_ctx = talloc(be_req, struct ad_subdomains_req_ctx);
to:
    req_ctx = talloc_zero(be_req, struct ad_subdomains_req_ctx);

So that we get a crash next time?

[Pavel Reichl]

On 09/24/2015 05:11 PM, Jakub Hrozek wrote:
> On Thu, Sep 24, 2015 at 05:05:20PM +0200, Pavel Reichl wrote:
> > Hello,
> >
> > please see simple patch attached.
> >
> > Thanks!
>
> Thanks, this would solve the bug, but can you also change the allocation
> of the structure from:
>      req_ctx = talloc(be_req, struct ad_subdomains_req_ctx);
> to:
>      req_ctx = talloc_zero(be_req, struct ad_subdomains_req_ctx);
>
> So that we get a crash next time?

Sure, done. I just don't think we would get the crash sooner. I think this actually makes explicit initialization to NULL somewhat redundant. Would you like me to remove the explicit initialization?
> sssd-devel mailing list
> [e-mail]
> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

[Lukas Slebodnik]

On (24/09/15 17:30), Pavel Reichl wrote:
> On 09/24/2015 05:11 PM, Jakub Hrozek wrote:
> > On Thu, Sep 24, 2015 at 05:05:20PM +0200, Pavel Reichl wrote:
> > > Hello,
> > >
> > > please see simple patch attached.
> > >
> > > Thanks!
> >
> > Thanks, this would solve the bug, but can you also change the allocation
> > of the structure from:
> >     req_ctx = talloc(be_req, struct ad_subdomains_req_ctx);
> > to:
> >     req_ctx = talloc_zero(be_req, struct ad_subdomains_req_ctx);
> >
> > So that we get a crash next time?
>
> Sure, done. I just don't think we would get the crash sooner. I think this actually makes explicit initialization to NULL somewhat redundant. Would you like me to remove the explicit initialization?
> > sssd-devel mailing list
> > [e-mail]
> > https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
> From df57de04abb0d5871bd5d802251e761c1daa1889 Mon Sep 17 00:00:00 2001
> From: Pavel Reichl 
> Date: Thu, 24 Sep 2015 11:03:12 -0400
> Subject: [PATCH] AD: inicialize root_domain_attrs field
>
> Resolves:
> https://fedorahosted.org/sssd/ticket/2805
>
> src/providers/ad/ad_subdomains.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/src/providers/ad/ad_subdomains.c b/src/providers/ad/ad_subdomains.c
> index 8ed3dab0995f78a16f4a7df2e729ea88a39a782c..c2a6544fb7f146058acee9baca9b0cc6ee50aa3f 100644
> --- a/src/providers/ad/ad_subdomains.c
> +++ b/src/providers/ad/ad_subdomains.c
> @@ -505,7 +505,7 @@ static void ad_subdomains_retrieve(struct ad_subdomains_ctx *ctx,
>     int dp_error = DP_ERR_FATAL;
>     int ret;
> req_ctx = talloc(be_req, struct ad_subdomains_req_ctx);
> req_ctx = talloc_zero(be_req, struct ad_subdomains_req_ctx);
> if (req_ctx == NULL) {
>    ret = ENOMEM;
>    goto done;
>
> @@ -519,6 +519,7 @@ static void ad_subdomains_retrieve(struct ad_subdomains_ctx *ctx,
>     req_ctx->root_id_ctx = NULL;
>     req_ctx->root_op = NULL;
>     req_ctx->root_domain = NULL;
> req_ctx->root_domain_attrs = NULL;
> req_ctx->reply_count = 0;
> req_ctx->reply = NULL;

There is a common agreement (at least from management side) that we want
to improve code coverage. This patch is great opportunity to add regression
test. We have a possibility to execute unit tests with valgrind in our CI
script. So in this case cmocka unit test should work better. You can
inspire in another similar test "test_ipa_subdom_server"

BTW I know there is a planned release for 1.13.1 but we are not in a hurry
and it can be postponed few days until test will be ready.

LS

[Jakub Hrozek]

On Fri, Sep 25, 2015 at 08:24:35AM +0200, Lukas Slebodnik wrote:
> On (24/09/15 17:30), Pavel Reichl wrote:
> > On 09/24/2015 05:11 PM, Jakub Hrozek wrote:
> > > On Thu, Sep 24, 2015 at 05:05:20PM +0200, Pavel Reichl wrote:
> > > > Hello,
> > > >
> > > > please see simple patch attached.
> > > >
> > > > Thanks!
> > >
> > > Thanks, this would solve the bug, but can you also change the allocation
> > > of the structure from:
> > >     req_ctx = talloc(be_req, struct ad_subdomains_req_ctx);
> > > to:
> > >     req_ctx = talloc_zero(be_req, struct ad_subdomains_req_ctx);
> > >
> > > So that we get a crash next time?
> >
> > Sure, done. I just don't think we would get the crash sooner. I think this actually makes explicit initialization to NULL somewhat redundant. Would you like me to remove the explicit initialization?
> > > sssd-devel mailing list
> > > [e-mail]
> > > https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
> > From df57de04abb0d5871bd5d802251e761c1daa1889 Mon Sep 17 00:00:00 2001
> > From: Pavel Reichl 
> > Date: Thu, 24 Sep 2015 11:03:12 -0400
> > Subject: [PATCH] AD: inicialize root_domain_attrs field
> >
> > Resolves:
> > https://fedorahosted.org/sssd/ticket/2805
> >
> > src/providers/ad/ad_subdomains.c | 3 ++-
> > 1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/src/providers/ad/ad_subdomains.c b/src/providers/ad/ad_subdomains.c
> > index 8ed3dab0995f78a16f4a7df2e729ea88a39a782c..c2a6544fb7f146058acee9baca9b0cc6ee50aa3f 100644
> > --- a/src/providers/ad/ad_subdomains.c
> > +++ b/src/providers/ad/ad_subdomains.c
> > @@ -505,7 +505,7 @@ static void ad_subdomains_retrieve(struct ad_subdomains_ctx *ctx,
> >     int dp_error = DP_ERR_FATAL;
> >     int ret;
> > req_ctx = talloc(be_req, struct ad_subdomains_req_ctx);
> > req_ctx = talloc_zero(be_req, struct ad_subdomains_req_ctx);
> > if (req_ctx == NULL) {
> >    ret = ENOMEM;
> >    goto done;
> >
> > @@ -519,6 +519,7 @@ static void ad_subdomains_retrieve(struct ad_subdomains_ctx *ctx,
> >     req_ctx->root_id_ctx = NULL;
> >     req_ctx->root_op = NULL;
> >     req_ctx->root_domain = NULL;
> > req_ctx->root_domain_attrs = NULL;
> > req_ctx->reply_count = 0;
> > req_ctx->reply = NULL;
>
> There is a common agreement (at least from management side) that we want
> to improve code coverage. This patch is great opportunity to add regression
> test. We have a possibility to execute unit tests with valgrind in our CI
> script. So in this case cmocka unit test should work better. You can
> inspire in another similar test "test_ipa_subdom_server"
>
> BTW I know there is a planned release for 1.13.1 but we are not in a hurry
> and it can be postponed few days until test will be ready.

We should release when we know downstream tests are all OK (except this
one). The test can be added later (as long as Pavel agrees to write the
patch).

The test_ipa_subdom_server test took me some time, that's why I don't
think we need to block the release over a test.

[Lukas Slebodnik]

On (25/09/15 10:09), Jakub Hrozek wrote:
> On Fri, Sep 25, 2015 at 08:24:35AM +0200, Lukas Slebodnik wrote:
> > On (24/09/15 17:30), Pavel Reichl wrote:
> > > On 09/24/2015 05:11 PM, Jakub Hrozek wrote:
> > > > On Thu, Sep 24, 2015 at 05:05:20PM +0200, Pavel Reichl wrote:
> > > > > Hello,
> > > > >
> > > > > please see simple patch attached.
> > > > >
> > > > > Thanks!
> > > >
> > > > Thanks, this would solve the bug, but can you also change the allocation
> > > > of the structure from:
> > > >     req_ctx = talloc(be_req, struct ad_subdomains_req_ctx);
> > > > to:
> > > >     req_ctx = talloc_zero(be_req, struct ad_subdomains_req_ctx);
> > > >
> > > > So that we get a crash next time?
> > >
> > > Sure, done. I just don't think we would get the crash sooner. I think this actually makes explicit initialization to NULL somewhat redundant. Would you like me to remove the explicit initialization?
> > > > sssd-devel mailing list
> > > > [e-mail]
> > > > https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
> > > From df57de04abb0d5871bd5d802251e761c1daa1889 Mon Sep 17 00:00:00 2001
> > > From: Pavel Reichl 
> > > Date: Thu, 24 Sep 2015 11:03:12 -0400
> > > Subject: [PATCH] AD: inicialize root_domain_attrs field
> > >
> > > Resolves:
> > > https://fedorahosted.org/sssd/ticket/2805
> > >
> > > src/providers/ad/ad_subdomains.c | 3 ++-
> > > 1 file changed, 2 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/src/providers/ad/ad_subdomains.c b/src/providers/ad/ad_subdomains.c
> > > index 8ed3dab0995f78a16f4a7df2e729ea88a39a782c..c2a6544fb7f146058acee9baca9b0cc6ee50aa3f 100644
> > > --- a/src/providers/ad/ad_subdomains.c
> > > +++ b/src/providers/ad/ad_subdomains.c
> > > @@ -505,7 +505,7 @@ static void ad_subdomains_retrieve(struct ad_subdomains_ctx *ctx,
> > >     int dp_error = DP_ERR_FATAL;
> > >     int ret;
> > > req_ctx = talloc(be_req, struct ad_subdomains_req_ctx);
> > > req_ctx = talloc_zero(be_req, struct ad_subdomains_req_ctx);
> > > if (req_ctx == NULL) {
> > >    ret = ENOMEM;
> > >    goto done;
> > >
> > > @@ -519,6 +519,7 @@ static void ad_subdomains_retrieve(struct ad_subdomains_ctx *ctx,
> > >     req_ctx->root_id_ctx = NULL;
> > >     req_ctx->root_op = NULL;
> > >     req_ctx->root_domain = NULL;
> > > req_ctx->root_domain_attrs = NULL;
> > > req_ctx->reply_count = 0;
> > > req_ctx->reply = NULL;
> >
> > There is a common agreement (at least from management side) that we want
> > to improve code coverage. This patch is great opportunity to add regression
> > test. We have a possibility to execute unit tests with valgrind in our CI
> > script. So in this case cmocka unit test should work better. You can
> > inspire in another similar test "test_ipa_subdom_server"
> >
> > BTW I know there is a planned release for 1.13.1 but we are not in a hurry
> > and it can be postponed few days until test will be ready.
>
> We should release when we know downstream tests are all OK (except this
> one). The test can be added later (as long as Pavel agrees to write the
> patch).

The experience from past says when a bug was fixed an patch was pushed
to master without test then it took months to write a test for bugfix.
Or it was not written at all. We need to change it.
If developers knows that patch will not be accepted without test and
the fix block the release then writing unit/integration test will have
the highest priority for him/her.
> The test_ipa_subdom_server test took me some time, that's why I don't
> think we need to block the release over a test.

We are not in hurry.

LS

[Jakub Hrozek]

On Fri, Sep 25, 2015 at 10:31:02AM +0200, Lukas Slebodnik wrote:
> > The test_ipa_subdom_server test took me some time, that's why I don't
> > think we need to block the release over a test.
>
> We are not in hurry.

Define "We". The Prominent North American Enterprise Linux Vendor
downstream kinda is in hurry.

[Jakub Hrozek]

On Fri, Sep 25, 2015 at 11:00:07AM +0200, Jakub Hrozek wrote:
> On Fri, Sep 25, 2015 at 10:31:02AM +0200, Lukas Slebodnik wrote:
> > > The test_ipa_subdom_server test took me some time, that's why I don't
> > > think we need to block the release over a test.
> >
> > We are not in hurry.
>
> Define "We". The Prominent North American Enterprise Linux Vendor
> downstream kinda is in hurry.

Since I broke the code in ece345a74cec793e6d970a4955beb3d4a05935b3 which
had no tests for the modified source, I should write the tests now:
    https://fedorahosted.org/sssd/ticket/2806

[Jakub Hrozek]

On Tue, Sep 29, 2015 at 09:25:13AM +0200, Jakub Hrozek wrote:
> On Fri, Sep 25, 2015 at 11:00:07AM +0200, Jakub Hrozek wrote:
> > On Fri, Sep 25, 2015 at 10:31:02AM +0200, Lukas Slebodnik wrote:
> > > > The test_ipa_subdom_server test took me some time, that's why I don't
> > > > think we need to block the release over a test.
> > >
> > > We are not in hurry.
> >
> > Define "We". The Prominent North American Enterprise Linux Vendor
> > downstream kinda is in hurry.
>
> Since I broke the code in ece345a74cec793e6d970a4955beb3d4a05935b3 which
> had no tests for the modified source, I should write the tests now:
>     https://fedorahosted.org/sssd/ticket/2806

ACK and I pushed the patch to master: 
    master: 101628a48d25ffae3b13c75d0b0b01577188c803

I just confused this patch and another one and set a wrong R-B :-/ sorry
about that.