On Wed, Jan 9, 2013 at 12:16 PM, Jarno Rajahalme
<jarno.rajaha...@nsn.com> wrote:
>
> On Jan 9, 2013, at 20:19 , ext Jesse Gross wrote:
>
>> On Wed, Jan 9, 2013 at 2:33 AM, Jarno Rajahalme <jarno.rajaha...@nsn.com>
>> wrote:
>>>
>>> On Jan 8, 2013, at 21:07 , ext Jesse Gross wrote:
>>>
>>>> On Tue, Jan 8, 2013 at 3:41 AM, Jarno Rajahalme <jarno.rajaha...@nsn.com>
>>>> wrote:
>>>>> Userspace code will need to keep on checking for both ENODEV and ENOENT as
>>>>> long as older kernel modules are around.
>>>>>
>>>>> Signed-off-by: Jarno Rajahalme <jarno.rajaha...@nsn.com>
>>>>
>>>> OVS userspace actually uses this distinction to determine whether it
>>>> is the datapath or the port that doesn't exist. For example, see
>>>> netdev_vport_set_config(). In general, it's also usually helpful to
>>>> have more information about the cause of the error as long as it
>>>> doesn't expose implementation details.
>>>
>>> Actually, it seems to me that netdev_vport_set_config() does the
>>> OVS_VPORT_CMD_SET based on the vport name only. In the "name" branch of the
>>> lookup_vport() ENODEV is returned if a matching vport cannot be found, i.e.
>>> ENOENT would not be returned to netdev_vport_set_config(). Also, it is the
>>> ENODEV value netdev_vport_set_config() looks for, not ENOENT. If
>>> netdev_vport_set_config() were to use ODP port number to refer to the
>>> vport, it would also need to check for ENOENT in addition to ENODEV.
>>>
>>> It is the mismatch between the name and port_no branches of lookup_vport()
>>> that triggered me to propose the change. Currently:
>>>
>>> If name is given,
>>> and no such port is found, return ENODEV
>>> if dp_ifindex is given but it does not match, return ENODEV
>>> else if port_no is given
>>> and dp (via dp_ifindex) cannot be found, return ENODEV
>>> if the dp has no port_no, return ENOENT
>>>
>>> To me this does not look right. A different value for similar error case
>>> ("valid datapath, no such port") is returned depending on whether the query
>>> is done with a name or a port_no.
>>
>> OK, that seems fair.
>>
>> In the commit message, you mentioned that userspace will need to keep
>> checking both ENOENT and ENODEV. However, I only see one use of
>> querying by number (in dpif-linux.c:report_loss() which doesn't care
>> about the actual error value) and only one use of ENOENT (in
>> dpif-linux.c:dpif_linux_is_internal_device() which queries by name).
>> Neither of these seem to be affected by this change or would require
>> any special care going forward. Is there any other case that you
>> noticed or were you only talking about future uses?
>
> No, my thinking was that if someone at userspace makes a difference
> between the two, they will need keep on doing it, since they might be
> talking with an older kernel module. Now that you checked that no-one
> actually really depends on the that behavior, the latter part of my commit
> message is moot.
OK, applied, thanks.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
