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

[Jesse Gross]

On Mon, Jan 10, 2011 at 4:27 PM, Ben Pfaff <blp at nicira.com> wrote:
> 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/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?

When I saw this I felt the need to look through all the places that
dpif_port was used to figure out what the correct semantics were, so I
don't think that it is a great API.  It doesn't seem too hard to fix,
so I would prefer that it is consistent.