hi Sam,
Thanks for incorporating the comments in the latest patch.
On Sep 25, 2014, at 2:44 PM, Samuel Ghinet <[email protected]>
wrote:
>> All of the vport commands are implemented in Vport.c. Pls. move this to that
>> file. Perhaps even the Vport dump. You can move all of them at once in a
>> subsequent commit.
> I know that. I was not sure of the approach I should take.
>
> The linux ovs driver does all the netlink communication part in datapath.c.
> Except for the flows, which is in flow_netlink.c as I remember.
> I had considered the same approach here. It may changed later the way you
> guys think it would look best.
It needs to be in one place - either Datapath.c or Vport.c. All the flow
related commands are in Flow.c, so, Vport.c might be a good idea. We can do
this later.
>> Are all the attributes mandatory? I'd think that one of
>> OVS_VPORT_ATTR_PORT_NO or OVS_VPORT_ATTR_NAME are mandatory. Rest are
>> optional.
> Oops, my bad. I think I had tested this function upon not last commit, where
> the "optional" field did not exist yet. I fixed it.
OK
>
>> I am not sure if 'STATUS_DATA_NOT_ACCEPTED' is the correct error code to be
>> returned here. The documentation says:
>> "{Data Not Accepted} The TDI client could not handle the data received
>> during an indication."
> I have STATUS_INVALID_PARAMETER for now. May not be the best value though.
None of these error codes map to anything meaningful in userspace. We can
revisit these when we update the userspace code. But, picking the closest match
is a good idea for now.
>> Returning a transactional error with ENODEV would be appropriate I think, or
>> you could return STATUS_INVALID_PARAMETER.
> No. ENODEV will mean that the vport does not exist. Reason why I cannot use
> that error code here.
> Later on we might need to consider a proper error value, either as
> transactional error or as error returned by DeviceIoControl, and apply this
> in one place for all netlink functions (all require the switch context).
> Hmmm, in our implementation we replied with NL_ERROR_PERM for this case.
> Perhaps it might be a good fit.
>
> Anyway, this approach we have:
> [CODE]
> OvsAcquireCtrlLock();
> if (!gOvsSwitchContext) {
> OvsReleaseCtrlLock();
> return STATUS_INVALID_PARAMETER;
> }
> OvsReleaseCtrlLock();
> [/CODE]
> Is not proper either, because we cannot guarantee that after
> OvsReleaseCtrlLock, the extension will not detach or that gOvsSwitchContext
> will not become NULL or the memory it points to, deallocated.
> We should normally handle the case where netlink commands are requested while
> the extension is detached, so that the detaching waits for all current
> netlink commands to finish, and that after the extension is detached, all
> OvsDeviceControl operations will fail.
> But this will be another issue.
Yes, making the usage of the CtrlLock more meaningful is on my list of things
to do.
-- Nithin
_______________________________________________
dev mailing list
[email protected]
http://openvswitch.org/mailman/listinfo/dev
