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.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
