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

From: Jinpu Wang <jinpu.wang@cloud.ionos.com>
To: Bart Van Assche <bvanassche@acm.org>
Cc: linux-block@vger.kernel.org, linux-rdma@vger.kernel.org,
	Jens Axboe <axboe@kernel.dk>,
	Christoph Hellwig <hch@infradead.org>,
	Sagi Grimberg <sagi@grimberg.me>,
	Leon Romanovsky <leon@kernel.org>,
	Doug Ledford <dledford@redhat.com>,
	Jason Gunthorpe <jgg@ziepe.ca>,
	Danil Kipnis <danil.kipnis@cloud.ionos.com>,
	Roman Penyaev <rpenyaev@suse.de>,
	Pankaj Gupta <pankaj.gupta@cloud.ionos.com>
Subject: Re: [PATCH v11 04/26] RDMA/rtrs: core: lib functions shared between client and server modules
Date: Mon, 30 Mar 2020 12:34:00 +0200	[thread overview]
Message-ID: <CAMGffE=oU=auw9Re3JcpBx2cap=6i4P0R__bcO4NnN+yW76b8w@mail.gmail.com> (raw)
In-Reply-To: <cad654ae-d6c9-882d-aeeb-d6871994d280@acm.org>

On Sat, Mar 28, 2020 at 5:26 AM Bart Van Assche <bvanassche@acm.org> wrote:
>
> 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'.
ok, will use u16.
>
> > +/**
> > + * 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.
how about just "port number"?
>
> > + * @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?
yes, should be "source".
>
> > + *
> > + * 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.
ok.
>
> > +                        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.
Yes, will update the comments.

>
> > +                     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?
Yes, will update the comment.
>
> Thanks,
>
> Bart.

Thanks Bart!