Обсуждение: cleanup patches for dshash

While working on the dynamic shared memory registry, I noticed a couple of
potential improvements for code that uses dshash tables.

* A couple of dshash_create() callers pass in 0 for the "void *arg"
  parameter, which seemed weird.  I incorrectly assumed this was some sort
  of flags parameter until I actually looked at the function signature.
  IMHO we should specify NULL here if arg is not used.  0001 makes this
  change.  This is admittedly a nitpick.

* There are no dshash compare/hash functions for string keys.  0002
  introduces some that simply forward to strcmp()/string_hash(), and it
  uses them for the DSM registry's dshash table.  This allows us to remove
  the hacky key padding code for lookups on this table.

Thoughts?

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

Nathan Bossart <nathandbossart@gmail.com> writes:

> While working on the dynamic shared memory registry, I noticed a couple of
> potential improvements for code that uses dshash tables.
>
> * A couple of dshash_create() callers pass in 0 for the "void *arg"
>   parameter, which seemed weird.  I incorrectly assumed this was some sort
>   of flags parameter until I actually looked at the function signature.
>   IMHO we should specify NULL here if arg is not used.  0001 makes this
>   change.  This is admittedly a nitpick.
>
> * There are no dshash compare/hash functions for string keys.  0002
>   introduces some that simply forward to strcmp()/string_hash(), and it
>   uses them for the DSM registry's dshash table.  This allows us to remove
>   the hacky key padding code for lookups on this table.
>
> Thoughts?

Both LGTM.

+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);


-- 
Best Regards
Andy Fan

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

On Sun, Jan 21, 2024 at 09:51:18PM -0600, Nathan Bossart wrote:
> 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...

I attempted to fix this in v2 of the patch set.

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

On Sun, Jan 21, 2024 at 11:07:15PM -0600, Nathan Bossart wrote:
> I attempted to fix this in v2 of the patch set.

If there are no objections, I plan to commit these patches early next week.

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

On Fri, Feb 23, 2024 at 03:52:16PM -0600, Nathan Bossart wrote:
> If there are no objections, I plan to commit these patches early next week.

Committed.

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

On Mon, Feb 26, 2024 at 03:55:10PM -0600, Nathan Bossart wrote:
> Committed.

I noticed that I forgot to update a couple of comments.  While fixing
those, I discovered additional oversights that have been around since 2017.
I plan to commit this shortly. 

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