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

From: Bart Van Assche <bvanassche@acm.org>
To: Jack Wang <jinpuwang@gmail.com>,
	linux-block@vger.kernel.org, linux-rdma@vger.kernel.org
Cc: axboe@kernel.dk, hch@infradead.org, sagi@grimberg.me,
	leon@kernel.org, dledford@redhat.com, jgg@ziepe.ca,
	danil.kipnis@cloud.ionos.com, jinpu.wang@cloud.ionos.com,
	rpenyaev@suse.de, pankaj.gupta@cloud.ionos.com
Subject: Re: [PATCH v9 04/25] RDMA/rtrs: core: lib functions shared between client and server modules
Date: Sat, 29 Feb 2020 16:47:12 -0800	[thread overview]
Message-ID: <91708531-46f2-3593-ae08-383a36feea5c@acm.org> (raw)
In-Reply-To: <20200221104721.350-5-jinpuwang@gmail.com>

On 2020-02-21 02:47, Jack Wang wrote:
> +	wr = (struct ib_recv_wr) {
> +	.wr_cqe  = &iu->cqe,
> +	.sg_list = &list,
> +	.num_sge = 1,
> +	};

The indentation of the above code looks weird to me.

> +	wr = (struct ib_recv_wr) {
> +	.wr_cqe  = cqe,
> +	};

Same comment here.

> +	wr = (struct ib_send_wr) {
> +	.wr_cqe     = &iu->cqe,
> +	.sg_list    = &list,
> +	.num_sge    = 1,
> +	.opcode     = IB_WR_SEND,
> +	.send_flags = IB_SEND_SIGNALED,
> +	};

And here.

> +	wr = (struct ib_rdma_wr) {
> +	.wr.wr_cqe	  = &iu->cqe,
> +	.wr.sg_list	  = sge,
> +	.wr.num_sge	  = num_sge,
> +	.rkey		  = rkey,
> +	.remote_addr	  = rdma_addr,
> +	.wr.opcode	  = IB_WR_RDMA_WRITE_WITH_IMM,
> +	.wr.ex.imm_data = cpu_to_be32(imm_data),
> +	.wr.send_flags  = flags,
> +	};

And here too.

> +	/*
> +	 * If one of the sges has 0 size, the operation will fail with an
> +	 * length error
> +	 */

"an length error" -> "a length error"?

> +	wr = (struct ib_send_wr) {
> +	.wr_cqe	= cqe,
> +	.send_flags	= flags,
> +	.opcode	= IB_WR_RDMA_WRITE_WITH_IMM,
> +	.ex.imm_data	= cpu_to_be32(imm_data),
> +	};

Please indent struct members.

> +int sockaddr_to_str(const struct sockaddr *addr, char *buf, size_t len)
> +{
> +	int cnt;
> +
> +	switch (addr->sa_family) {
> +	case AF_IB:
> +		cnt = scnprintf(buf, len, "gid:%pI6",
> +			&((struct sockaddr_ib *)addr)->sib_addr.sib_raw);
> +		return cnt;
> +	case AF_INET:
> +		cnt = scnprintf(buf, len, "ip:%pI4",
> +			&((struct sockaddr_in *)addr)->sin_addr);
> +		return cnt;
> +	case AF_INET6:
> +		cnt = scnprintf(buf, len, "ip:%pI6c",
> +			  &((struct sockaddr_in6 *)addr)->sin6_addr);
> +		return cnt;
> +	}
> +	cnt = scnprintf(buf, len, "<invalid address family>");
> +	pr_err("Invalid address family\n");
> +	return cnt;
> +}
> +EXPORT_SYMBOL(sockaddr_to_str);

Please remove the 'cnt' variable from the above function.

Thanks,

Bart.