[ovs-dev] [PATCH 1/2] datapath: Do not send success message when OVS_VPORT_CMD_SET command fails

Ansis Atteka aatteka at nicira.com
Wed Apr 4 17:44:54 UTC 2012


On Mon, Mar 26, 2012 at 11:21 AM, Jesse Gross <jesse at nicira.com> wrote:

> On Fri, Mar 23, 2012 at 6:04 PM, Ansis Atteka <aatteka at 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.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openvswitch.org/pipermail/ovs-dev/attachments/20120404/07c53f81/attachment-0003.html>


More information about the dev mailing list