[PATCH] Return error code from ipa_subdom_store

[Jakub Hrozek]

I don't think there is a reason to ignore error codes while storing
subdomains.

[Sumit Bose]

On Tue, Mar 05, 2013 at 12:20:12AM +0100, Jakub Hrozek wrote:
> I don't think there is a reason to ignore error codes while storing
> subdomains.

I see it the other way round, there is no reason to return an error if
storing a subdomain fails, because we cannot do anything about it. If an
error is returned here, the operation which triggered to subdomain
lookup, e.g. getpwnam() with a fully qualified name, will fail. But the
needed subdomain for this request might already be stored by a previous
sysdb_subdomain_store() call, so if we return EOK the request might
finish successfully.

For this reasons I prefer to ignore the error. But if you think it would
be better that the original request fails so that the error is not
unnoticed I wouldn't be against it.

bye,
Sumit
> From de913e37e2e9a379e8e323f758e139cc610aff59 Mon Sep 17 00:00:00 2001
> From: Jakub Hrozek 
> Date: Tue, 5 Mar 2013 00:15:35 +0100
> Subject: [PATCH] Return error code from ipa_subdom_store
>
> src/providers/ipa/ipa_subdomains.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/src/providers/ipa/ipa_subdomains.c b/src/providers/ipa/ipa_subdomains.c
> index 7d6e5958ed1e8ff9ea663c9e3bc754442a46f3c6..29ebd1fed1dec8b58119c4251c15c7d070a91fd8 100644
> --- a/src/providers/ipa/ipa_subdomains.c
> +++ b/src/providers/ipa/ipa_subdomains.c
> @@ -230,11 +230,13 @@ static errno_t ipa_subdom_store(struct sss_domain_info *domain,
>      ret = sysdb_subdomain_store(domain->sysdb, name, realm, flat, id);
>      if (ret) {
>          DEBUG(SSSDBG_OP_FAILURE, ("sysdb_subdomain_store failed.\n"));
>    goto done;
>
> }
>
> ret = EOK;
>
> done:
>      talloc_free(tmp_ctx);
> return EOK;
> return ret;
>
> }
> static errno_t
>
> 1.8.1.4
> sssd-devel mailing list
> [e-mail]
> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

[Jakub Hrozek]

On Tue, Mar 05, 2013 at 09:57:36AM +0100, Sumit Bose wrote:
> On Tue, Mar 05, 2013 at 12:20:12AM +0100, Jakub Hrozek wrote:
> > I don't think there is a reason to ignore error codes while storing
> > subdomains.
>
> I see it the other way round, there is no reason to return an error if
> storing a subdomain fails, because we cannot do anything about it. If an
> error is returned here, the operation which triggered to subdomain
> lookup, e.g. getpwnam() with a fully qualified name, will fail. But the
> needed subdomain for this request might already be stored by a previous
> sysdb_subdomain_store() call, so if we return EOK the request might
> finish successfully.
>
> For this reasons I prefer to ignore the error. But if you think it would
> be better that the original request fails so that the error is not
> unnoticed I wouldn't be against it.
>
> bye,
> Sumit

Hm, you're probably right. I would still prefer to at least report an
error with a DEBUG message and then return EOK or explicitly ignore the
return code in the caller. But I think the code has to explicitly say
that the error is being ignored on purpose, otherwise it's confusing.

Thank you for the review, I will prepare another patch.

[Jakub Hrozek]

On Tue, Mar 05, 2013 at 11:06:30AM +0100, Jakub Hrozek wrote:
> On Tue, Mar 05, 2013 at 09:57:36AM +0100, Sumit Bose wrote:
> > On Tue, Mar 05, 2013 at 12:20:12AM +0100, Jakub Hrozek wrote:
> > > I don't think there is a reason to ignore error codes while storing
> > > subdomains.
> >
> > I see it the other way round, there is no reason to return an error if
> > storing a subdomain fails, because we cannot do anything about it. If an
> > error is returned here, the operation which triggered to subdomain
> > lookup, e.g. getpwnam() with a fully qualified name, will fail. But the
> > needed subdomain for this request might already be stored by a previous
> > sysdb_subdomain_store() call, so if we return EOK the request might
> > finish successfully.
> >
> > For this reasons I prefer to ignore the error. But if you think it would
> > be better that the original request fails so that the error is not
> > unnoticed I wouldn't be against it.
> >
> > bye,
> > Sumit
>
> Hm, you're probably right. I would still prefer to at least report an
> error with a DEBUG message and then return EOK or explicitly ignore the
> return code in the caller. But I think the code has to explicitly say
> that the error is being ignored on purpose, otherwise it's confusing.
>
> Thank you for the review, I will prepare another patch.

Attached is a patch that returns errno but only warns in the callers.

[Pavel Březina]

On 03/06/2013 03:05 PM, Jakub Hrozek wrote:
> On Tue, Mar 05, 2013 at 11:06:30AM +0100, Jakub Hrozek wrote:
> > > On Tue, Mar 05, 2013 at 09:57:36AM +0100, Sumit Bose wrote:
> > > > > On Tue, Mar 05, 2013 at 12:20:12AM +0100, Jakub Hrozek wrote:
> > > > > > >I don't think there is a reason to ignore error codes while storing
> > > > > > >subdomains.
> > > > >
> > > > > I see it the other way round, there is no reason to return an error if
> > > > > storing a subdomain fails, because we cannot do anything about it. If an
> > > > > error is returned here, the operation which triggered to subdomain
> > > > > lookup, e.g. getpwnam() with a fully qualified name, will fail. But the
> > > > > needed subdomain for this request might already be stored by a previous
> > > > > sysdb_subdomain_store() call, so if we return EOK the request might
> > > > > finish successfully.
> > > > >
> > > > > For this reasons I prefer to ignore the error. But if you think it would
> > > > > be better that the original request fails so that the error is not
> > > > > unnoticed I wouldn't be against it.
> > > > >
> > > > > bye,
> > > > > Sumit
> > >
> > > Hm, you're probably right. I would still prefer to at least report an
> > > error with a DEBUG message and then return EOK or explicitly ignore the
> > > return code in the caller. But I think the code has to explicitly say
> > > that the error is being ignored on purpose, otherwise it's confusing.
> > >
> > > Thank you for the review, I will prepare another patch.
>
> Attached is a patch that returns errno but only warns in the callers.
> @@ -431,8 +433,11 @@ static errno_t ipa_subdomains_refresh(struct ipa_subdomains_ctx *ctx,
>               /* ok let's try to update it */
>               ret = ipa_subdom_store(domain, reply[c]);
>               if (ret) {
>            DEBUG(SSSDBG_OP_FAILURE, ("Failed to parse subdom data\n"));
>            goto done;
>            /* Nothing we can do about the erorr. Let's at least try

^ typo
>             * to reuse the existing domain
>             */
>            DEBUG(SSSDBG_MINOR_FAILURE, ("Failed to parse subdom data, "
>                  "will try to use cached subdomain\n"));
>         }
>         handled[c] = true;
>         h++;
>
> @@ -452,10 +457,13 @@ static errno_t ipa_subdomains_refresh(struct ipa_subdomains_ctx *ctx,
>           if (handled[c]) {
>               continue;
>           }
>    /* Nothing we can do about the erorr. Let's at least try

^ typo
>     * to reuse the existing domain.
>     */
>     ret = ipa_subdom_store(domain, reply[c]);
>     if (ret) {
>        DEBUG(SSSDBG_OP_FAILURE, ("Failed to parse subdom data\n"));
>        goto done;
>        DEBUG(SSSDBG_MINOR_FAILURE, ("Failed to parse subdom data, "
>              "will try to use cached subdomain\n"));
>     }
> }

Otherwise ack. Feel free to push it without re-reviewing.

[Jakub Hrozek]

On Wed, Mar 20, 2013 at 01:56:31PM +0100, Pavel Březina wrote:
> On 03/06/2013 03:05 PM, Jakub Hrozek wrote:
> > On Tue, Mar 05, 2013 at 11:06:30AM +0100, Jakub Hrozek wrote:
> > > > On Tue, Mar 05, 2013 at 09:57:36AM +0100, Sumit Bose wrote:
> > > > > > On Tue, Mar 05, 2013 at 12:20:12AM +0100, Jakub Hrozek wrote:
> > > > > > > >I don't think there is a reason to ignore error codes while storing
> > > > > > > >subdomains.
> > > > > >
> > > > > > I see it the other way round, there is no reason to return an error if
> > > > > > storing a subdomain fails, because we cannot do anything about it. If an
> > > > > > error is returned here, the operation which triggered to subdomain
> > > > > > lookup, e.g. getpwnam() with a fully qualified name, will fail. But the
> > > > > > needed subdomain for this request might already be stored by a previous
> > > > > > sysdb_subdomain_store() call, so if we return EOK the request might
> > > > > > finish successfully.
> > > > > >
> > > > > > For this reasons I prefer to ignore the error. But if you think it would
> > > > > > be better that the original request fails so that the error is not
> > > > > > unnoticed I wouldn't be against it.
> > > > > >
> > > > > > bye,
> > > > > > Sumit
> > > >
> > > > Hm, you're probably right. I would still prefer to at least report an
> > > > error with a DEBUG message and then return EOK or explicitly ignore the
> > > > return code in the caller. But I think the code has to explicitly say
> > > > that the error is being ignored on purpose, otherwise it's confusing.
> > > >
> > > > Thank you for the review, I will prepare another patch.
> >
> > Attached is a patch that returns errno but only warns in the callers.
> > @@ -431,8 +433,11 @@ static errno_t ipa_subdomains_refresh(struct ipa_subdomains_ctx *ctx,
> >              /* ok let's try to update it */
> >              ret = ipa_subdom_store(domain, reply[c]);
> >              if (ret) {
> >            DEBUG(SSSDBG_OP_FAILURE, ("Failed to parse subdom data\n"));
> >            goto done;
> >            /* Nothing we can do about the erorr. Let's at least try
>                                                ^ typo
>
> >             * to reuse the existing domain
> >             */
> >            DEBUG(SSSDBG_MINOR_FAILURE, ("Failed to parse subdom data, "
> >                  "will try to use cached subdomain\n"));
> >        }
> >        handled[c] = true;
> >        h++;
> >
> > @@ -452,10 +457,13 @@ static errno_t ipa_subdomains_refresh(struct ipa_subdomains_ctx *ctx,
> >          if (handled[c]) {
> >              continue;
> >          }
> >    /* Nothing we can do about the erorr. Let's at least try
>                                        ^ typo
>
> >     * to reuse the existing domain.
> >     */
> >    ret = ipa_subdom_store(domain, reply[c]);
> >    if (ret) {
> >        DEBUG(SSSDBG_OP_FAILURE, ("Failed to parse subdom data\n"));
> >        goto done;
> >        DEBUG(SSSDBG_MINOR_FAILURE, ("Failed to parse subdom data, "
> >              "will try to use cached subdomain\n"));
> >    }
> > }
>
> Otherwise ack. Feel free to push it without re-reviewing.

Fixed typo and pushed to master.