On Fri, Mar 23, 2012 at 6:04 PM, Ansis Atteka <aatt...@nicira.com> wrote:
> diff --git a/datapath/datapath.c b/datapath/datapath.c
> index d64fc32..daf7b69 100644
> --- a/datapath/datapath.c
> +++ b/datapath/datapath.c
> + if (!err) {
The difficult thing about this function is that it's possible to fail
in the middle without being able to revert (the upstream version
doesn't have this problem because it doesn't allow you to set the
address of ports). This means that we could have changed something
but now won't send a notification, which isn't good.
For the upstream version (and us when we change the tunneling code to
not need a MAC address) this solution makes sense. Even though I
don't believe that anyone other than OVS listens to OVS notifications,
it's good to get them correct.
I think that a good userspace implementation of Netlink should be able
to handle both the notification and error report. In fact I think
this is happening all the time on success because we pass both
NLM_F_ECHO and NLM_F_ACK. In that case though, both are interpreted
as success so there's no issue but it doesn't seem right to me.
> + reply = ovs_vport_cmd_build_info(vport, info->snd_pid,
> info->snd_seq,
> + OVS_VPORT_CMD_NEW);
> + if (IS_ERR(reply))
> + netlink_set_err(GENL_SOCK(sock_net(skb->sk)), 0,
> + ovs_dp_vport_multicast_group.id, err);
When reporting an error, it should be PTR_ERR(reply), otherwise you'll
always set 0 here.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
