* [PATCH] ksmbd: hide socket error message when ipv6 config is disable
@ 2022-09-27 21:51 Namjae Jeon
2022-09-28 15:25 ` Tom Talpey
0 siblings, 1 reply; 6+ messages in thread
From: Namjae Jeon @ 2022-09-27 21:51 UTC (permalink / raw)
To: linux-cifs; +Cc: smfrench, senozhatsky, tom, atteh.mailbox, Namjae Jeon
When ipv6 config is disable(CONFIG_IPV6 is not set), ksmbd fallback to
create ipv4 socket. User reported that this error message lead to
misunderstood some issue. Users have requested not to print this error
message that occurs even though there is no problem.
Signed-off-by: Namjae Jeon <linkinjeon@kernel.org>
---
fs/ksmbd/transport_tcp.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/fs/ksmbd/transport_tcp.c b/fs/ksmbd/transport_tcp.c
index 143bba4e4db8..9b35afcdcf0d 100644
--- a/fs/ksmbd/transport_tcp.c
+++ b/fs/ksmbd/transport_tcp.c
@@ -399,7 +399,8 @@ static int create_socket(struct interface *iface)
ret = sock_create(PF_INET6, SOCK_STREAM, IPPROTO_TCP, &ksmbd_socket);
if (ret) {
- pr_err("Can't create socket for ipv6, try ipv4: %d\n", ret);
+ if (ret != -EAFNOSUPPORT)
+ pr_err("Can't create socket for ipv6, try ipv4: %d\n", ret);
ret = sock_create(PF_INET, SOCK_STREAM, IPPROTO_TCP,
&ksmbd_socket);
if (ret) {
--
2.25.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] ksmbd: hide socket error message when ipv6 config is disable
2022-09-27 21:51 [PATCH] ksmbd: hide socket error message when ipv6 config is disable Namjae Jeon
@ 2022-09-28 15:25 ` Tom Talpey
2022-09-28 23:44 ` Namjae Jeon
2022-09-29 2:08 ` Sergey Senozhatsky
0 siblings, 2 replies; 6+ messages in thread
From: Tom Talpey @ 2022-09-28 15:25 UTC (permalink / raw)
To: Namjae Jeon, linux-cifs; +Cc: smfrench, senozhatsky, atteh.mailbox
On 9/27/2022 5:51 PM, Namjae Jeon wrote:
> When ipv6 config is disable(CONFIG_IPV6 is not set), ksmbd fallback to
> create ipv4 socket. User reported that this error message lead to
> misunderstood some issue. Users have requested not to print this error
> message that occurs even though there is no problem.
>
> Signed-off-by: Namjae Jeon <linkinjeon@kernel.org>
> ---
> fs/ksmbd/transport_tcp.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/fs/ksmbd/transport_tcp.c b/fs/ksmbd/transport_tcp.c
> index 143bba4e4db8..9b35afcdcf0d 100644
> --- a/fs/ksmbd/transport_tcp.c
> +++ b/fs/ksmbd/transport_tcp.c
> @@ -399,7 +399,8 @@ static int create_socket(struct interface *iface)
>
> ret = sock_create(PF_INET6, SOCK_STREAM, IPPROTO_TCP, &ksmbd_socket);
> if (ret) {
> - pr_err("Can't create socket for ipv6, try ipv4: %d\n", ret);
> + if (ret != -EAFNOSUPPORT)
> + pr_err("Can't create socket for ipv6, try ipv4: %d\n", ret);
Why not just eliminate the splat? The only real error seems to be
that IPv6 is not configured, which is undoubtedly intentional, and
in any case there's nothing to do about it. Suggesting to "try ipv4"
is kind of pointless, isn't it?
> ret = sock_create(PF_INET, SOCK_STREAM, IPPROTO_TCP,
> &ksmbd_socket);
> if (ret) {
The same question applies to IPv4 - socket creation is not something
that fails in general, and spraying the kernel log isn't particularly
useful toward fixing it. In any case, the error propagates back up
to the caller, right? Why wouldn't ksmbd.mountd do the reporting?
Tom.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] ksmbd: hide socket error message when ipv6 config is disable
2022-09-28 15:25 ` Tom Talpey
@ 2022-09-28 23:44 ` Namjae Jeon
2022-09-29 15:37 ` Tom Talpey
2022-09-29 2:08 ` Sergey Senozhatsky
1 sibling, 1 reply; 6+ messages in thread
From: Namjae Jeon @ 2022-09-28 23:44 UTC (permalink / raw)
To: Tom Talpey; +Cc: linux-cifs, smfrench, senozhatsky, atteh.mailbox
2022-09-29 0:25 GMT+09:00, Tom Talpey <tom@talpey.com>:
> On 9/27/2022 5:51 PM, Namjae Jeon wrote:
>> When ipv6 config is disable(CONFIG_IPV6 is not set), ksmbd fallback to
>> create ipv4 socket. User reported that this error message lead to
>> misunderstood some issue. Users have requested not to print this error
>> message that occurs even though there is no problem.
>>
>> Signed-off-by: Namjae Jeon <linkinjeon@kernel.org>
>> ---
>> fs/ksmbd/transport_tcp.c | 3 ++-
>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/fs/ksmbd/transport_tcp.c b/fs/ksmbd/transport_tcp.c
>> index 143bba4e4db8..9b35afcdcf0d 100644
>> --- a/fs/ksmbd/transport_tcp.c
>> +++ b/fs/ksmbd/transport_tcp.c
>> @@ -399,7 +399,8 @@ static int create_socket(struct interface *iface)
>>
>> ret = sock_create(PF_INET6, SOCK_STREAM, IPPROTO_TCP, &ksmbd_socket);
>> if (ret) {
>> - pr_err("Can't create socket for ipv6, try ipv4: %d\n", ret);
>> + if (ret != -EAFNOSUPPORT)
>> + pr_err("Can't create socket for ipv6, try ipv4: %d\n", ret);
>
> Why not just eliminate the splat? The only real error seems to be
> that IPv6 is not configured, which is undoubtedly intentional, and
No, It can return other errors.
> in any case there's nothing to do about it. Suggesting to "try ipv4"
> is kind of pointless, isn't it?
No, It is not bad to give info to users. users can check ksmbd
connection status using netstats.
>
>> ret = sock_create(PF_INET, SOCK_STREAM, IPPROTO_TCP,
>> &ksmbd_socket);
>> if (ret) {
>
> The same question applies to IPv4 - socket creation is not something
> that fails in general, and spraying the kernel log isn't particularly
> useful toward fixing it.
I don't understand what you are saying. Since it's not common, it print an error
and give the information to users.
> In any case, the error propagates back up
> to the caller, right? Why wouldn't ksmbd.mountd do the reporting?
Why does ksmbd.mountd appear here?
>
> Tom.
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] ksmbd: hide socket error message when ipv6 config is disable
2022-09-28 15:25 ` Tom Talpey
2022-09-28 23:44 ` Namjae Jeon
@ 2022-09-29 2:08 ` Sergey Senozhatsky
2022-09-29 2:48 ` Namjae Jeon
1 sibling, 1 reply; 6+ messages in thread
From: Sergey Senozhatsky @ 2022-09-29 2:08 UTC (permalink / raw)
To: Tom Talpey; +Cc: Namjae Jeon, linux-cifs, smfrench, senozhatsky, atteh.mailbox
On (22/09/28 11:25), Tom Talpey wrote:
> > diff --git a/fs/ksmbd/transport_tcp.c b/fs/ksmbd/transport_tcp.c
> > index 143bba4e4db8..9b35afcdcf0d 100644
> > --- a/fs/ksmbd/transport_tcp.c
> > +++ b/fs/ksmbd/transport_tcp.c
> > @@ -399,7 +399,8 @@ static int create_socket(struct interface *iface)
> > ret = sock_create(PF_INET6, SOCK_STREAM, IPPROTO_TCP, &ksmbd_socket);
> > if (ret) {
> > - pr_err("Can't create socket for ipv6, try ipv4: %d\n", ret);
> > + if (ret != -EAFNOSUPPORT)
> > + pr_err("Can't create socket for ipv6, try ipv4: %d\n", ret);
>
> Why not just eliminate the splat? The only real error seems to be
> that IPv6 is not configured, which is undoubtedly intentional, and
> in any case there's nothing to do about it. Suggesting to "try ipv4"
> is kind of pointless, isn't it?
Yeah, that pr_err() sounds like a suggestion, but in fact it's not.
It meant to say "ipv6 socket creation failed, fallback to ipv4".
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] ksmbd: hide socket error message when ipv6 config is disable
2022-09-29 2:08 ` Sergey Senozhatsky
@ 2022-09-29 2:48 ` Namjae Jeon
0 siblings, 0 replies; 6+ messages in thread
From: Namjae Jeon @ 2022-09-29 2:48 UTC (permalink / raw)
To: Sergey Senozhatsky; +Cc: Tom Talpey, linux-cifs, smfrench, atteh.mailbox
2022-09-29 11:08 GMT+09:00, Sergey Senozhatsky <senozhatsky@chromium.org>:
> On (22/09/28 11:25), Tom Talpey wrote:
>> > diff --git a/fs/ksmbd/transport_tcp.c b/fs/ksmbd/transport_tcp.c
>> > index 143bba4e4db8..9b35afcdcf0d 100644
>> > --- a/fs/ksmbd/transport_tcp.c
>> > +++ b/fs/ksmbd/transport_tcp.c
>> > @@ -399,7 +399,8 @@ static int create_socket(struct interface *iface)
>> > ret = sock_create(PF_INET6, SOCK_STREAM, IPPROTO_TCP,
>> > &ksmbd_socket);
>> > if (ret) {
>> > - pr_err("Can't create socket for ipv6, try ipv4: %d\n", ret);
>> > + if (ret != -EAFNOSUPPORT)
>> > + pr_err("Can't create socket for ipv6, try ipv4: %d\n", ret);
>>
>> Why not just eliminate the splat? The only real error seems to be
>> that IPv6 is not configured, which is undoubtedly intentional, and
>> in any case there's nothing to do about it. Suggesting to "try ipv4"
>> is kind of pointless, isn't it?
>
> Yeah, that pr_err() sounds like a suggestion, but in fact it's not.
> It meant to say "ipv6 socket creation failed, fallback to ipv4".
I agree that it's better to change it to "fallback to ipv4" instead of
"try ipv4".
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] ksmbd: hide socket error message when ipv6 config is disable
2022-09-28 23:44 ` Namjae Jeon
@ 2022-09-29 15:37 ` Tom Talpey
0 siblings, 0 replies; 6+ messages in thread
From: Tom Talpey @ 2022-09-29 15:37 UTC (permalink / raw)
To: Namjae Jeon; +Cc: linux-cifs, smfrench, senozhatsky, atteh.mailbox
On 9/28/2022 7:44 PM, Namjae Jeon wrote:
> 2022-09-29 0:25 GMT+09:00, Tom Talpey <tom@talpey.com>:
>> On 9/27/2022 5:51 PM, Namjae Jeon wrote:
>>> When ipv6 config is disable(CONFIG_IPV6 is not set), ksmbd fallback to
>>> create ipv4 socket. User reported that this error message lead to
>>> misunderstood some issue. Users have requested not to print this error
>>> message that occurs even though there is no problem.
>>>
>>> Signed-off-by: Namjae Jeon <linkinjeon@kernel.org>
>>> ---
>>> fs/ksmbd/transport_tcp.c | 3 ++-
>>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/fs/ksmbd/transport_tcp.c b/fs/ksmbd/transport_tcp.c
>>> index 143bba4e4db8..9b35afcdcf0d 100644
>>> --- a/fs/ksmbd/transport_tcp.c
>>> +++ b/fs/ksmbd/transport_tcp.c
>>> @@ -399,7 +399,8 @@ static int create_socket(struct interface *iface)
>>>
>>> ret = sock_create(PF_INET6, SOCK_STREAM, IPPROTO_TCP, &ksmbd_socket);
>>> if (ret) {
>>> - pr_err("Can't create socket for ipv6, try ipv4: %d\n", ret);
>>> + if (ret != -EAFNOSUPPORT)
>>> + pr_err("Can't create socket for ipv6, try ipv4: %d\n", ret);
>>
>> Why not just eliminate the splat? The only real error seems to be
>> that IPv6 is not configured, which is undoubtedly intentional, and
> No, It can return other errors.
In extremely exceptional circumstances, like the system out of memory
or a system without sockets configured. I just think these are not
worth raising in such a way. There is a handful of other pr_err's in
the same routine that I feel the same way about. They all seem to be
targeted at a developer, rather than being useful operationally.
>> in any case there's nothing to do about it. Suggesting to "try ipv4"
>> is kind of pointless, isn't it?
> No, It is not bad to give info to users. users can check ksmbd
> connection status using netstats.
>>
>>> ret = sock_create(PF_INET, SOCK_STREAM, IPPROTO_TCP,
>>> &ksmbd_socket);
>>> if (ret) {
>>
>> The same question applies to IPv4 - socket creation is not something
>> that fails in general, and spraying the kernel log isn't particularly
>> useful toward fixing it.
> I don't understand what you are saying. Since it's not common, it print an error
> and give the information to users.
>> In any case, the error propagates back up
>> to the caller, right? Why wouldn't ksmbd.mountd do the reporting?
> Why does ksmbd.mountd appear here?
I mention it because ksmbd.mountd is what loads the configuration and
starts the kernel processes. So it's logical that it would be the one
to report errors.
The present approach is something like "start the daemon", "if any
issues, sudo dmesg and see what you find." I, um, don't think that's
production-ready.
I'll go with your change for now.
Acked-by: Tom Talpey <tom@talpey.com>
>>
>> Tom.
>>
>
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2022-09-29 15:40 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-09-27 21:51 [PATCH] ksmbd: hide socket error message when ipv6 config is disable Namjae Jeon
2022-09-28 15:25 ` Tom Talpey
2022-09-28 23:44 ` Namjae Jeon
2022-09-29 15:37 ` Tom Talpey
2022-09-29 2:08 ` Sergey Senozhatsky
2022-09-29 2:48 ` Namjae Jeon
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).