diff mbox series

[V1] net: qrtr: ns: Ignore ENODEV failures in ns

Message ID 1703153211-3717-1-git-send-email-quic_sarannya@quicinc.com
State Superseded
Headers show
Series [V1] net: qrtr: ns: Ignore ENODEV failures in ns | expand

Commit Message

Sarannya S Dec. 21, 2023, 10:06 a.m. UTC 
From: Chris Lew <quic_clew@quicinc.com>

Ignore the ENODEV failures returned by kernel_sendmsg(). These errors
indicate that either the local port has been closed or the remote has
gone down. Neither of these scenarios are fatal and will eventually be
handled through packets that are later queued on the control port.

Signed-off-by: Chris Lew <quic_clew@quicinc.com>
Signed-off-by: Sarannya Sasikumar <quic_sarannya@quicinc.com>
---
 net/qrtr/ns.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

Comments

Simon Horman Dec. 23, 2023, 1:56 p.m. UTC | #1 
[Dropped bjorn.andersson@kernel.org, as the correct address seems
 to be andersson@kernel.org, which is already in the CC list.
 kernel.org rejected sending this email without that update.]

On Thu, Dec 21, 2023 at 03:36:50PM +0530, Sarannya S wrote:
> From: Chris Lew <quic_clew@quicinc.com>
> 
> Ignore the ENODEV failures returned by kernel_sendmsg(). These errors
> indicate that either the local port has been closed or the remote has
> gone down. Neither of these scenarios are fatal and will eventually be
> handled through packets that are later queued on the control port.
> 
> Signed-off-by: Chris Lew <quic_clew@quicinc.com>
> Signed-off-by: Sarannya Sasikumar <quic_sarannya@quicinc.com>
> ---
>  net/qrtr/ns.c | 11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/net/qrtr/ns.c b/net/qrtr/ns.c
> index abb0c70..8234339 100644
> --- a/net/qrtr/ns.c
> +++ b/net/qrtr/ns.c
> @@ -157,7 +157,7 @@ static int service_announce_del(struct sockaddr_qrtr *dest,
>  	msg.msg_namelen = sizeof(*dest);
>  
>  	ret = kernel_sendmsg(qrtr_ns.sock, &msg, &iv, 1, sizeof(pkt));
> -	if (ret < 0)
> +	if (ret < 0 && ret != -ENODEV)
>  		pr_err("failed to announce del service\n");
>  
>  	return ret;

Hi,

The caller of service_announce_del() ignores it's return value.
So the only action on error is the pr_err() call above, and so
with this patch -ENODEV is indeed ignored.

However, I wonder if it would make things clearer to the reader (me?)
if the return type of service_announce_del was updated void. Because
as things stand -ENODEV may be returned, which implies something might
handle that, even though it doe not.

The above notwithstanding, this change looks good to me.

Reviewed-by: Simon Horman <horms@kernel.org>

...
Simon Horman Dec. 23, 2023, 1:58 p.m. UTC | #2 
[Dropped bjorn.andersson@kernel.org, as the correct address seems
 to be andersson@kernel.org, which is already in the CC list.
 kernel.org rejected sending this email without that update.]

On Thu, Dec 21, 2023 at 03:36:51PM +0530, Sarannya S wrote:
> When a 'DEL_CLIENT' message is received from the remote, the corresponding
> server port gets deleted. A DEL_SERVER message is then announced for this
> server. As part of handling the subsequent DEL_SERVER message, the name-
> server attempts to delete the server port which results in a '-ENOENT' error.
> The return value from server_del() is then propagated back to qrtr_ns_worker,
> causing excessive error prints.
> To address this, return 0 from control_cmd_del_server() without checking the
> return value of server_del(), since the above scenario is not an error case
> and hence server_del() doesn't have any other error return value.
> 
> Signed-off-by: Sarannya Sasikumar <quic_sarannya@quicinc.com>

Thanks,

I have a suggestion below. But that notwithstanding this change
looks good to me.

Reviewed-by: Simon Horman <horms@kernel.org>

> ---
>  net/qrtr/ns.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/net/qrtr/ns.c b/net/qrtr/ns.c
> index b1db0b5..abb0c70 100644
> --- a/net/qrtr/ns.c
> +++ b/net/qrtr/ns.c
> @@ -512,7 +512,9 @@ static int ctrl_cmd_del_server(struct sockaddr_qrtr *from,
>  	if (!node)
>  		return -ENOENT;
>  
> -	return server_del(node, port, true);
> +	server_del(node, port, true);
> +
> +	return 0;
>  }

With this change the return value of server_del() now seems to be
ignored by all callers. Perhaps it would make sense to update it
to return void?

>  
>  static int ctrl_cmd_new_lookup(struct sockaddr_qrtr *from,
> -- 
> 2.7.4
> 
>
Chris Lew Dec. 27, 2023, 12:20 a.m. UTC | #3 
On 12/23/2023 5:56 AM, Simon Horman wrote:
> [Dropped bjorn.andersson@kernel.org, as the correct address seems
>   to be andersson@kernel.org, which is already in the CC list.
>   kernel.org rejected sending this email without that update.]
> 
> On Thu, Dec 21, 2023 at 03:36:50PM +0530, Sarannya S wrote:
>> From: Chris Lew <quic_clew@quicinc.com>
>>
>> Ignore the ENODEV failures returned by kernel_sendmsg(). These errors
>> indicate that either the local port has been closed or the remote has
>> gone down. Neither of these scenarios are fatal and will eventually be
>> handled through packets that are later queued on the control port.
>>
>> Signed-off-by: Chris Lew <quic_clew@quicinc.com>
>> Signed-off-by: Sarannya Sasikumar <quic_sarannya@quicinc.com>
>> ---
>>   net/qrtr/ns.c | 11 +++++++----
>>   1 file changed, 7 insertions(+), 4 deletions(-)
>>
>> diff --git a/net/qrtr/ns.c b/net/qrtr/ns.c
>> index abb0c70..8234339 100644
>> --- a/net/qrtr/ns.c
>> +++ b/net/qrtr/ns.c
>> @@ -157,7 +157,7 @@ static int service_announce_del(struct sockaddr_qrtr *dest,
>>   	msg.msg_namelen = sizeof(*dest);
>>   
>>   	ret = kernel_sendmsg(qrtr_ns.sock, &msg, &iv, 1, sizeof(pkt));
>> -	if (ret < 0)
>> +	if (ret < 0 && ret != -ENODEV)
>>   		pr_err("failed to announce del service\n");
>>   
>>   	return ret;
> 
> Hi,
> 
> The caller of service_announce_del() ignores it's return value.
> So the only action on error is the pr_err() call above, and so
> with this patch -ENODEV is indeed ignored.
> 
> However, I wonder if it would make things clearer to the reader (me?)
> if the return type of service_announce_del was updated void. Because
> as things stand -ENODEV may be returned, which implies something might
> handle that, even though it doe not.
> 
> The above notwithstanding, this change looks good to me.
> 
> Reviewed-by: Simon Horman <horms@kernel.org>
> 
> ...

Hi Simon, thanks for the review and suggestion. We weren't sure whether 
we should change the function prototype on these patches on the chance 
that there will be something that listens and handles this in the 
future. I think it's a good idea to change it to void and we can change 
it back if there is such a usecase in the future.
Simon Horman Jan. 4, 2024, 9:23 a.m. UTC | #4 
On Tue, Dec 26, 2023 at 04:20:03PM -0800, Chris Lew wrote:
> 
> 
> On 12/23/2023 5:56 AM, Simon Horman wrote:
> > [Dropped bjorn.andersson@kernel.org, as the correct address seems
> >   to be andersson@kernel.org, which is already in the CC list.
> >   kernel.org rejected sending this email without that update.]
> > 
> > On Thu, Dec 21, 2023 at 03:36:50PM +0530, Sarannya S wrote:
> > > From: Chris Lew <quic_clew@quicinc.com>
> > > 
> > > Ignore the ENODEV failures returned by kernel_sendmsg(). These errors
> > > indicate that either the local port has been closed or the remote has
> > > gone down. Neither of these scenarios are fatal and will eventually be
> > > handled through packets that are later queued on the control port.
> > > 
> > > Signed-off-by: Chris Lew <quic_clew@quicinc.com>
> > > Signed-off-by: Sarannya Sasikumar <quic_sarannya@quicinc.com>
> > > ---
> > >   net/qrtr/ns.c | 11 +++++++----
> > >   1 file changed, 7 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/net/qrtr/ns.c b/net/qrtr/ns.c
> > > index abb0c70..8234339 100644
> > > --- a/net/qrtr/ns.c
> > > +++ b/net/qrtr/ns.c
> > > @@ -157,7 +157,7 @@ static int service_announce_del(struct sockaddr_qrtr *dest,
> > >   	msg.msg_namelen = sizeof(*dest);
> > >   	ret = kernel_sendmsg(qrtr_ns.sock, &msg, &iv, 1, sizeof(pkt));
> > > -	if (ret < 0)
> > > +	if (ret < 0 && ret != -ENODEV)
> > >   		pr_err("failed to announce del service\n");
> > >   	return ret;
> > 
> > Hi,
> > 
> > The caller of service_announce_del() ignores it's return value.
> > So the only action on error is the pr_err() call above, and so
> > with this patch -ENODEV is indeed ignored.
> > 
> > However, I wonder if it would make things clearer to the reader (me?)
> > if the return type of service_announce_del was updated void. Because
> > as things stand -ENODEV may be returned, which implies something might
> > handle that, even though it doe not.
> > 
> > The above notwithstanding, this change looks good to me.
> > 
> > Reviewed-by: Simon Horman <horms@kernel.org>
> > 
> > ...
> 
> Hi Simon, thanks for the review and suggestion. We weren't sure whether we
> should change the function prototype on these patches on the chance that
> there will be something that listens and handles this in the future. I think
> it's a good idea to change it to void and we can change it back if there is
> such a usecase in the future.

Hi Chris,

yes, I think that would be a good approach.
diff mbox series

Patch

diff --git a/net/qrtr/ns.c b/net/qrtr/ns.c
index abb0c70..8234339 100644
--- a/net/qrtr/ns.c
+++ b/net/qrtr/ns.c
@@ -157,7 +157,7 @@  static int service_announce_del(struct sockaddr_qrtr *dest,
 	msg.msg_namelen = sizeof(*dest);
 
 	ret = kernel_sendmsg(qrtr_ns.sock, &msg, &iv, 1, sizeof(pkt));
-	if (ret < 0)
+	if (ret < 0 && ret != -ENODEV)
 		pr_err("failed to announce del service\n");
 
 	return ret;
@@ -188,7 +188,7 @@  static void lookup_notify(struct sockaddr_qrtr *to, struct qrtr_server *srv,
 	msg.msg_namelen = sizeof(*to);
 
 	ret = kernel_sendmsg(qrtr_ns.sock, &msg, &iv, 1, sizeof(pkt));
-	if (ret < 0)
+	if (ret < 0 && ret != -ENODEV)
 		pr_err("failed to send lookup notification\n");
 }
 
@@ -207,6 +207,9 @@  static int announce_servers(struct sockaddr_qrtr *sq)
 	xa_for_each(&node->servers, index, srv) {
 		ret = service_announce_new(sq, srv);
 		if (ret < 0) {
+			if (ret == -ENODEV)
+				continue;
+
 			pr_err("failed to announce new service\n");
 			return ret;
 		}
@@ -369,7 +372,7 @@  static int ctrl_cmd_bye(struct sockaddr_qrtr *from)
 		msg.msg_namelen = sizeof(sq);
 
 		ret = kernel_sendmsg(qrtr_ns.sock, &msg, &iv, 1, sizeof(pkt));
-		if (ret < 0) {
+		if (ret < 0 && ret != -ENODEV) {
 			pr_err("failed to send bye cmd\n");
 			return ret;
 		}
@@ -443,7 +446,7 @@  static int ctrl_cmd_del_client(struct sockaddr_qrtr *from,
 		msg.msg_namelen = sizeof(sq);
 
 		ret = kernel_sendmsg(qrtr_ns.sock, &msg, &iv, 1, sizeof(pkt));
-		if (ret < 0) {
+		if (ret < 0 && ret != -ENODEV) {
 			pr_err("failed to send del client cmd\n");
 			return ret;
 		}