Re: [PATCH] ksmbd: hide socket error message when ipv6 config is disable

[Tom Talpey <tom@talpey.com>]

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.
>>
>