[PATCH v3 1/6] rdma_cm: add rdma_reject_msg() helper function

[Steve Wise]

> -----Original Message-----
> From: Bart Van Assche [mailto:bart.vanassche at sandisk.com]
> Sent: Tuesday, October 25, 2016 12:58 PM
> To: Steve Wise; dledford at redhat.com; sean.hefty at intel.com
> Cc: linux-rdma at vger.kernel.org; linux-nvme at lists.infradead.org;
> sagi at grimberg.me; hch at lst.de; axboe at fb.com; santosh.shilimkar at oracle.com
> Subject: Re: [PATCH v3 1/6] rdma_cm: add rdma_reject_msg() helper function
> 
> On 10/24/2016 12:09 PM, Steve Wise wrote:
> > +	[IB_CM_REJ_RDC_NOT_EXIST]		= "rdc not exist",
> 
> Please change this into "RDC does not exist".
>

ok
 
> > +	[IB_CM_REJ_INVALID_GID]			= "invalid gid",
> > +	[IB_CM_REJ_INVALID_LID]			= "invalid lid",
> > +	[IB_CM_REJ_INVALID_SL]			= "invalid sl",
> > +	[IB_CM_REJ_INVALID_TRAFFIC_CLASS]	= "invalid traffic class",
> > +	[IB_CM_REJ_INVALID_HOP_LIMIT]		= "invalid hop limit",
> > +	[IB_CM_REJ_INVALID_PACKET_RATE]		= "invalid packet rate",
> > +	[IB_CM_REJ_INVALID_ALT_GID]		= "invalid alt gid",
> > +	[IB_CM_REJ_INVALID_ALT_LID]		= "invalid alt lid",
> > +	[IB_CM_REJ_INVALID_ALT_SL]		= "invalid alt sl",
> > +	[IB_CM_REJ_INVALID_ALT_TRAFFIC_CLASS]	= "invalid alt traffic class",
> > +	[IB_CM_REJ_INVALID_ALT_HOP_LIMIT]	= "invalid alt hop limit",
> > +	[IB_CM_REJ_INVALID_ALT_PACKET_RATE]	= "invalid alt packet rate",
> > +	[IB_CM_REJ_PORT_CM_REDIRECT]		= "port cm redirect",
> > +	[IB_CM_REJ_PORT_REDIRECT]		= "port redirect",
> > +	[IB_CM_REJ_INVALID_MTU]			= "invalid mtu",
> > +	[IB_CM_REJ_INSUFFICIENT_RESP_RESOURCES]	= "insufficient resp
> resources",
> > +	[IB_CM_REJ_CONSUMER_DEFINED]		= "consumer defined",
> > +	[IB_CM_REJ_INVALID_RNR_RETRY]		= "invalid rnr retry",
> > +	[IB_CM_REJ_DUPLICATE_LOCAL_COMM_ID]	= "duplicate local comm
> id",
> > +	[IB_CM_REJ_INVALID_CLASS_VERSION]	= "invalid class version",
> > +	[IB_CM_REJ_INVALID_FLOW_LABEL]		= "invalid flow label",
> > +	[IB_CM_REJ_INVALID_ALT_FLOW_LABEL]	= "invalid alt flow label",
> 
> Other error messages capitalize GID, LID, CM, ID, RNR and SL so please
> do this here too.
> 

ok

> > +const char *__attribute_const__ ibcm_reject_msg(int reason)
> > +{
> > +	size_t index = reason;
> > +
> > +	if (index >= ARRAY_SIZE(ibcm_rej_reason_strs) ||
> > +	    !ibcm_rej_reason_strs[index])
> > +		return "unrecognized reason";
> > +	else
> > +		return ibcm_rej_reason_strs[index];
> > +}
> 
> Please consider using positive logic - this means negating the
> if-condition and swapping the if- and else-parts.

yea that might be even clearer.

> 
> > +const char *__attribute_const__ rdma_reject_msg(struct rdma_cm_id *id,
> > +						int reason)
> > +{
> > +	if (rdma_ib_or_roce(id->device, id->port_num))
> > +		return ibcm_reject_msg(reason);
> > +
> > +	if (rdma_protocol_iwarp(id->device, id->port_num))
> > +		return iwcm_reject_msg(reason);
> > +
> > +	WARN_ON_ONCE(1);
> > +	return "unrecognized reason";
> 
> Have you considered to return "unrecognized transport" here instead?
> 

I can do that.

> > +const char *__attribute_const__ iwcm_reject_msg(int reason)
> > +{
> > +	size_t index;
> > +
> > +	/* iWARP uses negative errnos */
> > +	index = -reason;
> > +
> > +	if (index >= ARRAY_SIZE(iwcm_rej_reason_strs) ||
> > +	    !iwcm_rej_reason_strs[index])
> > +		return "unrecognized reason";
> > +	else
> > +		return iwcm_rej_reason_strs[index];
> > +}
> 
> Also for this function, please consider using positive logic.
> 
> Thanks,
> 
> Bart.