[SSSD] [PATCH] Return error code from ipa_subdom_store

Jakub Hrozek jhrozek at redhat.com
Wed Mar 20 13:39:54 UTC 2013


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.



More information about the sssd-devel mailing list