Michael Paquier <michael@paquier.xyz> writes:
> [ v3-0001-Improve-error-reporting-for-cryptohashes.patch ]
This is looking pretty solid to me. Just a couple of nitpicks:
* In most places you initialize variables holding error strings to NULL:
+ const char *logdetail = NULL;
but there are three or so spots that don't, eg PerformRadiusTransaction.
They should be consistent. (Even if there's no actual bug, I'd be
unsurprised to see Coverity carp about the inconsistency.)
* The comments for md5_crypt_verify and plain_crypt_verify claim that
the error string is "optionally" stored, but I don't see anything
optional about it. To me, "optional" would imply coding like
if (logdetail)
*logdetail = errstr;
which we don't have here, and I don't think we need it. But the
comments need adjustment. (They were wrong before too, but no
time like the present to clean them up.)
* I'd be inclined to just drop the existing comments like
- * We do not bother setting logdetail for any pg_md5_encrypt failure
- * below: the only possible error is out-of-memory, which is unlikely, and
- * if it did happen adding a psprintf call would only make things worse.
rather than modify them. Neither the premise nor the conclusion
of these comments is accurate anymore. (I think that the psprintf
they are talking about is the one that will happen inside elog.c
to construct an errdetail string. Yeah, there's some risk there,
but I think it's minimal because of the fact that we preallocate
some space in ErrorContext.)
Other than those things, I think v3 is good to go.
regards, tom lane