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.