Em dom., 14 de fev. de 2021 às 08:22, Michael Paquier <michael@paquier.xyz> escreveu:
On Sat, Feb 13, 2021 at 09:33:48PM -0300, Ranier Vilela wrote: > Em sáb., 13 de fev. de 2021 às 20:32, Michael Paquier <michael@paquier.xyz> > escreveu: > >> You are missing the point here. What you are proposing here would not >> be backpatched. However, reusing the same words as upthread, this has >> a cost in terms of *future* maintenance. In short, any *future* >> potential bug fix that would require to be backpatched in need of >> using this function or touching its area would result in a conflict. > > Ok. +1 for back-patching.
Please take the time to read again my previous email.
And also, please take the time to actually test patches you send, because the list of things getting broken is impressive. At least you make sure that the internals of cryptohash.c generate an error as they should because of those incorrect sizes :)
git diff --check complains, in various places.
@@ -330,7 +330,8 @@ SendBackupManifest(backup_manifest_info *manifest) * twice. */ manifest->still_checksumming = false; - if (pg_cryptohash_final(manifest->manifest_ctx, checksumbuf) < 0) + if (pg_cryptohash_final(manifest->manifest_ctx, checksumbuf, + sizeof(checksumbuf) - 1) < 0) elog(ERROR, "failed to finalize checksum of backup manifest"); This breaks backup manifests, due to an incorrect calculation.
Bad habits.
sizeof - 1, I use with strings.
I think that in pg_checksum_final() we had better save the digest length in "retval" before calling pg_cryptohash_final(), and use it for the size passed down.
pg_checksum_final I would like to see it like this:
case CHECKSUM_TYPE_SHA224: retval = PG_SHA224_DIGEST_LENGTH; break; case CHECKSUM_TYPE_SHA256: retval = PG_SHA256_DIGEST_LENGTH; break; case CHECKSUM_TYPE_SHA384: retval = PG_SHA384_DIGEST_LENGTH; break; case CHECKSUM_TYPE_SHA512: retval = PG_SHA512_DIGEST_LENGTH; break;
default:
return -1;
}
if (pg_cryptohash_final(context->raw_context.c_sha2, output, retval) < 0) return -1;