[ovs-dev] [worker v2 12/14] smap: New function smap_steal().

Ben Pfaff blp at nicira.com
Wed Jul 18 17:51:10 UTC 2012


On Wed, Jul 11, 2012 at 04:46:41PM -0500, Ethan Jackson wrote:
> > +/* Deletes 'node' from 'sh'.  Neither the node's key nor its value is freed;
> > + * instead, ownership is transferred to the caller.  Returns the node's key. */
> 
> This should say from 'smap'

Thanks, fixed.

> > +char *
> > +smap_steal(struct smap *smap, struct smap_node *node)
> > +{
> > +    char *key = node->key;
> > +
> > +    hmap_remove(&smap->map, &node->node);
> > +    free(node);
> > +    return key;
> > +}
> > +
> 
> I think the definition of this function is a bit misleading in a way
> that could cause memory leaks if someone isn't careful.  Basically,
> the only way to free node->value is to keep a pointer to it before
> calling this function, otherwise node (containing the only pointer to
> node->value) is dead.  This seems fine to me, but it's strange that
> you have to deal with value in this way, while key is helpfully
> returned by the function.  I'm afraid callers are going to forget to
> free value entirely causing leaks.
> 
> I can think of two possibly better implementations:
> 1) Return Nothing.  This would make the handling of key and value
> consistent, reminding callers that both need to be dealt with.
> 
> 2) Change the prototype to:
> void
> smap_steal(struct smap *smap, struct smap *node,
>            char **key, char **value);
> 
> If key is NULL, node->key is freed, otherwise node->key is put into
> key. Same with value.
> 
> What do you think?

I think you're right.

I took your choice 2.



More information about the dev mailing list