On Mon, Mar 26, 2012 at 11:21 AM, Jesse Gross <je...@nicira.com> wrote:
> 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.
>
Would this be an improvement, if we would send notification only when
ovs_vport_set_options() function succeeded (irrelevant whether
change_vport() succeeded)? Because as far as I understand notifications
should be sent only when something was actually changed (at least
partially)?
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.
>
One more thing I spotted is that ovs_vport_cmd_set() function is using
OVS_VPORT_CMD_NEW instead of OVS_VPORT_CMD_SET in their
notifications. This does not seem correct to me? I believe I saw the
same issue in some other places as well.
> 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.
>
Would it be a more correct behavior for user-space to wait till the NETLINK
reply where nlmsghdr.type==2 arrives, instead of relying on notifications to
figure out whether request succeeded or not?
> > + 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.
>
Ok.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
