Re: cleanup patches for dshash

On Mon, Jan 22, 2024 at 10:28:42AM +0800, Andy Fan wrote:
> Both LGTM.

Thanks for looking.

> +dshash_strcmp(const void *a, const void *b, size_t size, void *arg)
> +{
> + Assert(strlen((const char *) a) < size);
> + Assert(strlen((const char *) b) < size);
> +
>
> Do you think the below change will be more explicitly?
>
> #define DSMRegistryNameSize 64
>
> DSMRegistryEntry
> {
> char name[DSMRegistryNameSize];
>
> }
>
> Assert(strlen((const char *) a) < DSMRegistryNameSize);

This is effectively what it's doing already. These functions are intended
to be generic so that they could be used with other dshash tables with
string keys, possibly with different sizes.

I did notice a cfbot failure [0]. After a quick glance, I'm assuming this
is caused by the memcpy() in insert_into_bucket(). Even if the string is
short, it will copy the maximum key size, which is bad. So, 0002 is
totally broken at the moment, and we'd need to teach insert_into_bucket()
to strcpy() instead for string keys to fix it. Perhaps we could add a
field to dshash_parameters for this purpose...

[0] https://api.cirrus-ci.com/v1/artifact/task/5124848070950912/log/src/test/modules/test_dsm_registry/log/postmaster.log

--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com