> On 6 Nov 2026, at 03:11, Grigorev Jurij <[email protected]> wrote:
> Attached is v3, rebased on current master.
Thanks for the updated patch. This looks mostly ready, I just have a few
comments.
if (checksum_length != 0)
pg_checksum_init(&checksum_ctx,
CHECKSUM_TYPE_NONE);
- else
- pg_checksum_init(&checksum_ctx, checksum_type);
+ else if (pg_checksum_init(&checksum_ctx, checksum_type)
< 0)
+ pg_fatal("could not initialize checksum of file
\"%s\"",
+ ofullpath);
Nitpick, the conditionals in if and else if are unrelated, I would prefer this
to be broken up to improve readability something like the below. Also, an
assertion added to the TYPE_NONE case would be good.
if (checksum_length != 0)
{
ret = pg_checksum_init(&checksum_ctx, CHECKSUM_TYPE_NONE);
Assert(ret == 0);
}
else
{
ret = pg_checksum_init(&checksum_ctx, checksum_type);
if (ret < 0)
pg_fatal("could not initialize checksum of file \"%s\"", ofullpath);
}
This sketch might trigger a set-never-used compiler warning so it might not be
practical.
+test_checksum_helper$(X): $(TEST_OBJS) | submake-libpgport
submake-libpgfeutils
+ $(CC) $(CFLAGS) $^ $(LDFLAGS) $(LDFLAGS_EX) $(LIBS) -o $@$(X)
The (X) extension should not be in the rule name.
case CHECKSUM_TYPE_CRC32C:
FIN_CRC32C(context->raw_context.c_crc32c);
retval = sizeof(pg_crc32c);
memcpy(output, &context->raw_context.c_crc32c, retval);
- break;
+ return retval;
Not the fault of this patch, and mostly a nit-pick, but I think we should clear
.c_crc32c after the memcpy and set it to zero. This would make it consistent
with the sha2 types which with this sets the raw_context member to NULL (for
different reasons of course). That would be a separate patch though.
+command_exit_is(
+ ['test_checksum_helper'],
+ 0,
+ 'checksum contexts are cleaned up after cryptohash failures');
To make the test output easier to interpret and more in line with other test,
the helper application should be called once per test case.
--
Daniel Gustafsson
