Re: [PATCH v11 04/26] RDMA/rtrs: core: lib functions shared between client and server modules

[Bart Van Assche <bvanassche@acm.org>]

On 2020-03-20 05:16, Jack Wang wrote:
> +/**
> + * rtrs_str_to_sockaddr() - Convert rtrs address string to sockaddr
> + * @addr:	String representation of an addr (IPv4, IPv6 or IB GID):
> + *              - "ip:192.168.1.1"
> + *              - "ip:fe80::200:5aee:feaa:20a2"
> + *              - "gid:fe80::200:5aee:feaa:20a2"
> + * @len:        String address length
> + * @port:	Destination port
> + * @dst:	Destination sockaddr structure
> + *
> + * Returns 0 if conversion successful. Non-zero on error.
> + */
> +static int rtrs_str_to_sockaddr(const char *addr, size_t len,
> +				 short port, struct sockaddr_storage *dst)
> +{
> +	if (strncmp(addr, "gid:", 4) == 0) {
> +		return rtrs_str_gid_to_sockaddr(addr + 4, len - 4, port, dst);
> +	} else if (strncmp(addr, "ip:", 3) == 0) {
> +		char port_str[8];
> +		char *cpy;
> +		int err;
> +
> +		snprintf(port_str, sizeof(port_str), "%u", port);
> +		cpy = kstrndup(addr + 3, len - 3, GFP_KERNEL);
> +		err = cpy ? inet_pton_with_scope(&init_net, AF_UNSPEC,
> +						 cpy, port_str, dst) : -ENOMEM;
> +		kfree(cpy);
> +
> +		return err;
> +	}
> +	return -EPROTONOSUPPORT;
> +}

Please use 'u16' or 'uint16_t' for port numbers instead of 'short'.

> +/**
> + * rtrs_addr_to_sockaddr() - convert path string "src,dst" to sockaddreses
> + * @str:	string containing source and destination addr of a path
> + *		separated by comma. I.e. "ip:1.1.1.1,ip:1.1.1.2". If str
> + *		contains only one address it's considered to be destination.
> + * @len:	string length
> + * @port:	will be set to port.
                ^^^^^^^^^^^^^^^^^^^
What does this mean? Please make comments easy to comprehend.

> + * @addr:	will be set to the source/destination address or to NULL
> + *		if str doesn't contain any sorce address.
                                           ^^^^^
Is this perhaps a typo?

> + *
> + * Returns zero if conversion successful. Non-zero otherwise.
> + */
> +int rtrs_addr_to_sockaddr(const char *str, size_t len, short port,
                                                          ^^^^^
I think most kernel code uses type u16 for port numbers.

> +			   struct rtrs_addr *addr)
> +{
> +	const char *d;
> +
> +	d = strchr(str, ',');
> +	if (!d)
> +		d = strchr(str, '@');
> +	if (d) {
> +		if (rtrs_str_to_sockaddr(str, d - str, 0, addr->src))
                                                      ^^^
Does this mean that the @port argument only applies to the destination
address? If so, please mention this in the comment above this function.

> +			return -EINVAL;
> +		d += 1;
> +		len -= d - str;
> +		str  = d;
> +
> +	} else {
> +		addr->src = NULL;
> +	}
> +	return rtrs_str_to_sockaddr(str, len, port, addr->dst);
> +}
> +EXPORT_SYMBOL(rtrs_addr_to_sockaddr);

So this function either accepts ',' or '@' as separator between source
and destination address? Shouldn't that be mentioned in the comment
block above the function?

Thanks,

Bart.