* [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 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
* 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
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).