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

Ben Pfaff blp at nicira.com
Tue Sep 8 20:42:40 UTC 2015


On Tue, Sep 01, 2015 at 08:32:12PM -0400, Russell Bryant wrote:
> 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 had the same thoughts when I was writing this.  My first thought was
that it should be written so that it is impossible to assign it to a
non-"const" struct hmap.  However, I don't think that's actually
possible because it's always possible to convert a "const" literal to a
non-const one.  That is,
        int x = (const int) 123;
is valid, and so would be:
        struct hmap map = (const struct hmap) { ... };

Still, the intent here is that the hmap target should be const.  If that
is followed, then calling hmap_destroy() will produce a compiler error
message.  If that is not followed, then hmap_destroy() becomes a no-op
(because the initializer sets hmap.buckets to &hmap.one, and in that
case hmap_destroy() does nothing) but smap_destroy() should cause a
segfault due to freeing non-allocated storage, so I think the problem
would be found on the first test.

My first name for this was SMAP_CONST_INIT1, but it was too long in my
opinion; it forces lines elsewhere to be split.  Do you like
SMAP_CONST1?

> I'm just nit picking, though ... I think it's still an overall net benefit.
> 
> Acked-by: Russell Bryant <rbryant at redhat.com>

Thanks.  I'll apply this once the discussion seems to have concluded.



More information about the dev mailing list