[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