Re: [PATCH] net/9p/fd: change port to char *

[asmadeus@codewreck.org]

Joshua Murphy wrote on Fri, Jan 24, 2025 at 09:14:34PM +0000:
> The port variable in the option structs is changed to char * to
> remove the conversions from u16 to char *.
> 
> Signed-off-by: Joshua Murphy <joshuamurphy@posteo.net>
> ---
>  include/net/9p/client.h |  2 +-
>  net/9p/trans_fd.c       | 14 ++++++--------
>  2 files changed, 7 insertions(+), 9 deletions(-)
> 
> diff --git a/include/net/9p/client.h b/include/net/9p/client.h
> index 4f785098c..62be2073e 100644
> --- a/include/net/9p/client.h
> +++ b/include/net/9p/client.h
> @@ -114,7 +114,7 @@ struct p9_client {
>  			int wfd;
>  		} fd;
>  		struct {
> -			u16 port;
> +			char *port;
>  			bool privport;
>  
>  		} tcp;
> diff --git a/net/9p/trans_fd.c b/net/9p/trans_fd.c
> index 2fea50bf0..2874c7348 100644
> --- a/net/9p/trans_fd.c
> +++ b/net/9p/trans_fd.c
> @@ -31,7 +31,7 @@
>  
>  #include <linux/syscalls.h> /* killme */
>  
> -#define P9_PORT 564
> +#define P9_PORT "564"
>  #define MAX_SOCK_BUF (1024*1024)
>  #define MAXPOLLWADDR	2
>  
> @@ -49,7 +49,7 @@ static struct p9_trans_module p9_fd_trans;
>  struct p9_fd_opts {
>  	int rfd;
>  	int wfd;
> -	u16 port;
> +	char *port;
>  	bool privport;
>  };
>  
> @@ -746,8 +746,8 @@ static int p9_fd_cancelled(struct p9_client *client, struct p9_req_t *req)
>  static int p9_fd_show_options(struct seq_file *m, struct p9_client *clnt)
>  {
>  	if (clnt->trans_mod == &p9_tcp_trans) {
> -		if (clnt->trans_opts.tcp.port != P9_PORT)
> -			seq_printf(m, ",port=%u", clnt->trans_opts.tcp.port);
> +		if (!strcmp(clnt->trans_opts.tcp.port, P9_PORT))

Daniel pointed out at the inverted check (thank you for these reviews!),
but it also needs a null check, because fd/fd_unix variants will not be
setting this field so the strcmp will crash with trans=fd or trans=unix

> +			seq_printf(m, ",port=%s", clnt->trans_opts.tcp.port);
>  	} else if (clnt->trans_mod == &p9_fd_trans) {
>  		if (clnt->trans_opts.fd.rfd != ~0)
>  			seq_printf(m, ",rfd=%u", clnt->trans_opts.fd.rfd);
> @@ -804,7 +804,7 @@ static int parse_opts(char *params, struct p9_fd_opts *opts)
>  		}
>  		switch (token) {
>  		case Opt_port:
> -			opts->port = option;
> +			opts->port = args[0].from;

This is a substring from tmp_options that will be freed when parsinig is
finished, so we'll need to kstrdup() it here, which means we'll need to
free it when we're done with options... So, err, at the end of
p9_fd_create which doesn't copy it to the client, and in p9_fd_close for
tcp success (stored in client), and in p9_fd_create_tcp on error
(fd_close not called), and I _think_ that's about it...

I didn't quite estimate the clean up path would be this messy, this
"simple cleanup" is ending up being more complicated than the originial
version so I'm thinking we're better off keeping the int -- what do you
think?

-- 
Dominique Martinet | Asmadeus