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.