[ovs-dev] [netlink v3 16/16] datapath: Change vport type from string to integer enumeration.

[Ben Pfaff]

On Mon, Jan 10, 2011 at 03:20:50PM -0500, Jesse Gross wrote:
> On Wed, Dec 29, 2010 at 7:56 PM, Ben Pfaff <blp at nicira.com> wrote:
> > diff --git a/datapath/vport.c b/datapath/vport.c
> > index 6c053ac..862d170 100644
> > --- a/datapath/vport.c
> > +++ b/datapath/vport.c
> > @@ -748,7 +748,7 @@ const char *vport_get_name(const struct vport *vport)
> > ?* Retrieves the type of the given device. ?Either RTNL lock or rcu_read_lock
> > ?* must be held for the entire duration that the type is in use.
> > ?*/
> > -const char *vport_get_type(const struct vport *vport)
> > +enum odp_vport_type vport_get_type(const struct vport *vport)
> 
> The comment above this function is no longer really accurate - it's
> not necessary to hold a lock after the function call because we're not
> returning a pointer into private data.

Thanks, I deleted the phrase "for the entire duration that the type is
in use".

> > diff --git a/lib/dpif-linux.c b/lib/dpif-linux.c
> > index 34ef33d..ce55312 100644
> > --- a/lib/dpif-linux.c
> > +++ b/lib/dpif-linux.c
> > @@ -308,9 +330,8 @@ dpif_linux_port_query__(const struct dpif *dpif, uint32_t port_no,
> > ? ? ? ? /* A vport named 'port_name' exists but in some other datapath. ?*/
> > ? ? ? ? return ENOENT;
> > ? ? } else {
> > - ? ? ? ?translate_vport_type_to_netdev_type(&odp_port);
> > ? ? ? ? dpif_port->name = xstrdup(odp_port.devname);
> > - ? ? ? ?dpif_port->type = xstrdup(odp_port.type);
> > + ? ? ? ?dpif_port->type = xstrdup(vport_type_to_netdev_type(&odp_port));
> > ? ? ? ? dpif_port->port_no = odp_port.port;
> > ? ? ? ? return 0;
> > ? ? }
> > @@ -359,9 +380,8 @@ dpif_linux_port_dump_next(const struct dpif *dpif, void *state,
> > ? ? } else if (odp_port->devname[0] == '\0') {
> > ? ? ? ? return EOF;
> > ? ? } else {
> > - ? ? ? ?translate_vport_type_to_netdev_type(odp_port);
> > ? ? ? ? dpif_port->name = odp_port->devname;
> > - ? ? ? ?dpif_port->type = odp_port->type;
> > + ? ? ? ?dpif_port->type = (char *) vport_type_to_netdev_type(odp_port);
> 
> I know this was introduced in a previous patch but it concerns me that
> the strings in dpif_port need to be freed in some cases but not in
> others.  It appears that the right thing is being done now but it
> seems error prone.

Hmm, I thought about this but the use cases of iteration versus
retrieving a particular port seemed distinct enough to me that the
difference in semantics would not be too confusing.  How strongly do you
feel about it?

> For this patch though:
> Acked-by: Jesse Gross <jesse at nicira.com>

Thanks.