On Thu, Jan 17, 2019 at 4:03 AM Joe Orton <jor...@redhat.com> wrote:
> On Wed, Jan 16, 2019 at 12:02:01PM -0600, William A Rowe Jr wrote:
> > Doesn't this simply gloss over an underlying defect?
> >
> > [...]
> > if (apr_dbm_fetch(f, key, &val) == APR_SUCCESS && val.dptr) {
> > *value = apr_pstrmemdup(pool, val.dptr, val.dsize);
> > }
> >
> > apr_dbm_close(f);
> >
> > return rv;
> > }
> >
> > Shouldn't we capture and return the failure code from apr_dbm_fetch here?
>
> The error code from the failed lookup is not an interesting result, I
> assume because it can only fail one way. That the lookup failed is
> reflected by returning with success *value set to NULL.
>
> I had this change in my wc from looking at clang/coverity static
> analysis results so I assume it was flagged up somewhere but can't
> remember exactly. It seems like a harmless change.
>
Thanks for the reminder, yes... most providers will return only found
or not found; not found is not an error. However some providers may
fault, e.g. DB corrupt or storage interrupted or some other very rare
edge case. APR provided to return this info, whether the underlying
provider does is another question.
I'm concerned if clang/coverity flagged the existing code, since that
suggests some edge case that may not optimize correctly.
I withdraw my objection if we layer on the following comment to
avoid future confusion/research;
/* NOT FOUND is not an error case; this is indicated by NULL result.
* Treat all NULL lookup/error results as success for the simple case
* of auth credential lookup, these are DECLINED in both cases.
*/
Does that sound like a resolution Joe?
