Michael Paquier <michael@xxxxxxxxxxx> 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
