[ovs-dev] [PATCH] smap: New macro SMAP_INIT1 for initializing immutable 1-member smaps.

Russell Bryant rbryant at redhat.com
Wed Sep 2 00:32:12 UTC 2015


On 09/01/2015 04:35 PM, Ben Pfaff wrote:
> Reviewing the ovn-controller code I started to notice a common pattern:
> 
>     struct smap ext_ids = SMAP_INITIALIZER(&ext_ids);
>     smap_add(&ext_ids, "ovn-patch-port", network);
>     ovsrec_port_set_external_ids(port, &ext_ids);
>     smap_destroy(&ext_ids);
> 
> This seemed like a bit too much code for something as simple as
> initializing an smap with a single key-value pair.  This commit allows the
> code to be reduced to just:
> 
>     const struct smap ids = SMAP_INIT1(&ids, "ovn-patch-port", network);
>     ovsrec_port_set_external_ids(port, &ids);
> 
> This new form also eliminates multiple memory allocation and free
> operations, but I doubt that has any real effect on performance;
> the primary goal here is code readability.

I like that the code is so much shorter. However, I'm not sure it's any
more readable to me. The main thing that bothers me is that
smap_destroy() is now only used *some* of the time, where it used to be
nice and consistent.

I wonder if there's something we could do with the macro name that would
make it more clear that it doesn't need to be destroyed.  SMAP_LOCAL_INIT1?

I'm just nit picking, though ... I think it's still an overall net benefit.

Acked-by: Russell Bryant <rbryant at redhat.com>

-- 
Russell Bryant



More information about the dev mailing list