| Message ID | 20220927215151.6931-1-linkinjeon@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ksmbd: hide socket error message when ipv6 config is disable | expand |
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.
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. >
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".
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". >
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. >> >
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) {
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(-)