[ovs-dev] [PATCH] datapath: Change the single ENOENT return value to ENODEV...
jarno.rajahalme at nsn.com
Wed Jan 9 20:16:40 UTC 2013
On Jan 9, 2013, at 20:19 , ext Jesse Gross wrote:
> On Wed, Jan 9, 2013 at 2:33 AM, Jarno Rajahalme <jarno.rajahalme at 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.rajahalme at 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.rajahalme at 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.
More information about the dev