Hi Johan, >>>>>>> The convention of the L2CAP code is to return EFAULT when a CID in the >>>>>>> request packet is invalid. This patch fixes the l2cap_config_req to use >>>>>>> that error code instead of ENOENT. >>>>>>> >>>>>>> Signed-off-by: Johan Hedberg <johan.hedberg@xxxxxxxxx> >>>>>>> --- >>>>>>> net/bluetooth/l2cap_core.c | 2 +- >>>>>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>>>>> >>>>>>> diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c >>>>>>> index 9ec1561f..d134501 100644 >>>>>>> --- a/net/bluetooth/l2cap_core.c >>>>>>> +++ b/net/bluetooth/l2cap_core.c >>>>>>> @@ -3978,7 +3978,7 @@ static inline int l2cap_config_req(struct l2cap_conn *conn, >>>>>>> >>>>>>> chan = l2cap_get_chan_by_scid(conn, dcid); >>>>>>> if (!chan) >>>>>>> - return -ENOENT; >>>>>>> + return -EFAULT; >>>>>> >>>>>> do we really want to use EFAULT. That is normally only used if a >>>>>> memory copy from userspace to kernel or vice versa fails. >>>>> >>>>> EFAULT seemed to be the most common error code in this case, but now >>>>> that I did some research it seems it's mostly from newish AMP related >>>>> code. Would you be ok if I just switch all places to use ENOENT then? >>>> >>>> EFAULT is really for memory address errors. It is a pretty bad idea to use CID errors. >>>> >>>> I am fine with keep using ENOENT if we used that before. If that is >>>> what the original code used, then lets keep using it. Otherwise we >>>> better find a more descriptive error. >>> >>> If we ignore new AMP code there's only one "legacy" reference for this >>> kind of situation which is l2cap_config_req. Since it has always used >>> ENOENT for a failed CID lookup I'll follow that lead and use the same >>> error for all places that need it. >> >> have a look at EBADRQC, EBADSLT, EBADR or EBADE. They might be a >> better choice. > > I'd rather have a consensus here first before resending the entire patch > set yet another time because of this issue (which, let's face it, is not > about any public API but consistency inside a single c-file). > > As quite often with trying to map POSIX error codes to our own domain > we've ended up here with trying to find the "least bad" option in the > absence of a single obviously correct one. > > EBADRQC sounds like it's not related to PDU parameters but the request > code of the PDU. EBADSLT is "invalid slot", so if we consider the CID to > be a "slot" this might be an option. EBADR talks about a "request > descriptor" which I don't quite understand how it's different from a > "request code". EBADE means "invalid exchange" whereas this isn't really > about an exchange but just an incorrect CID in a PDU. > > I agree that ENOENT isn't perfect either since, even though internally > this is about a failed lookup, externally this is just the peer sending > us an invalid parameter value. Therefore, we could even consider EINVAL > if it wasn't for the fact that it's already used e.g. when we receive an > unknown opcode. > > In the end I'm willing to accept pretty much any of the proposed options > since there really isn't a single right answer to this and since this is > not about public API that we really need to stick to once we decide > which value to use. The important thing is that the code is kept > consistent regarding the use of this error. I have nothing against ENOENT. I however am a bit concerned about that we accidentally use ENOENT in some other case and end up overloading its meaning. So just for the sake of making this unique, I would consider going with EBADSLT. I am not married to this at all. However I wanted to have this talked through so that we understand the whole picture. If I would know the perfect answer, I would have just forced you to use it ;) If you want to keep using ENOENT, then I will sign off your current patches. Regards Marcel -- To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
