[SSSD] [PATCH] Return error code from ipa_subdom_store

Pavel Březina pbrezina at redhat.com
Wed Mar 20 12:56:31 UTC 2013


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.





More information about the sssd-devel mailing list