[ovs-dev] [PATCH 4/6] vswitch: Use "ipsec_gre" vport instead of "gre" with "other_config"

Justin Pettit jpettit at nicira.com
Thu Dec 23 02:17:15 UTC 2010


On Dec 22, 2010, at 3:29 PM, Jesse Gross wrote:

> On Wed, Dec 22, 2010 at 3:04 AM, Justin Pettit <jpettit at nicira.com> wrote:
>>                 if vals["ipsec_cert"]:
>>                     ipsec.ipsec_cert_update(vals["local_ip"],
>>                             vals["remote_ip"], vals["ipsec_cert"])
>> -                elif vals["ipsec_psk"]:
>> +                else vals["ipsec_psk"]:
>>                     ipsec.ipsec_psk_update(vals["local_ip"],
>>                             vals["remote_ip"], vals["ipsec_psk"])
> 
> Should that else still have vals["ipsec_psk"] after it?

I guess I never ran this intermediate commit.  :-)  Thanks.

>> +#define TNL_F_IS_IPSEC         (1 << 8) /* Traffic is IPsec encrypted. */
> 
> TNL_F_IPSEC would be more consistent with the others and shorter.

Okay, changed.

>> +    } else if (!strcmp(type, "gre")) {
>> +        struct tnl_port_config config;
>> +
>> +        memcpy(&config, port->config, sizeof config);
> 
> Again, I'm not sure why this is being copied.

Again, fixed.

>> @@ -254,7 +268,7 @@ dpif_linux_port_add(struct dpif *dpif, struct netdev *netdev,
>>     memset(&port, 0, sizeof port);
>>     strncpy(port.devname, name, sizeof port.devname);
>>     strncpy(port.type, type, sizeof port.type);
>> -    translate_netdev_type_to_vport_type(port.type, sizeof port.type);
>> +    translate_netdev_type_to_vport_type(&port);
>>     netdev_vport_get_config(netdev, port.config);
> 
> Nothing needs this now but it might make sense to put the call to
> netdev_vport_get_config() before the translate call.  This way we
> don't have to worry about uninitialized memory if we need the config.

Sounds good.  Changed.

>> diff --git a/lib/odp-util.c b/lib/odp-util.c
>> index e1ea976..8aeb98d 100644
>> --- a/lib/odp-util.c
>> +++ b/lib/odp-util.c
>> @@ -220,6 +220,7 @@ void
>>  format_odp_port_type(struct ds *ds, const struct odp_port *p)
>>  {
>>     if (!strcmp(p->type, "gre")
>> +            || !strcmp(p->type, "ipsec_gre")
>>             || !strcmp(p->type, "capwap")) {
>>         struct tnl_port_config config;
> 
> This won't properly translate the type to ipsec_gre.

Why not?  It's already been switched from "gre" with TNL_F_IPSEC set to "ipsec_gre".  Maybe I'm not understanding what you're saying...

Thanks for the reviews!

--Justin






More information about the dev mailing list