Re: [PATCH 2/2] nvmet: remove workarounds for gcc bug wrt unnamed fields in initializers

[Niklas Cassel <Niklas.Cassel@wdc.com>]

On Thu, Jun 18, 2020 at 03:23:21PM +0000, Chaitanya Kulkarni wrote:
> On 6/18/20 7:32 AM, Niklas Cassel wrote:
> >   drivers/nvme/target/rdma.c | 23 ++++++++++++-----------
> >   1 file changed, 12 insertions(+), 11 deletions(-)
> > 
> > diff --git a/drivers/nvme/target/rdma.c b/drivers/nvme/target/rdma.c
> > index 6731e0349480..85c6ff0b0e44 100644
> > --- a/drivers/nvme/target/rdma.c
> > +++ b/drivers/nvme/target/rdma.c
> > @@ -1535,19 +1535,20 @@ static int nvmet_rdma_cm_accept(struct rdma_cm_id *cm_id,
> >   		struct nvmet_rdma_queue *queue,
> >   		struct rdma_conn_param *p)
> >   {
> > -	struct rdma_conn_param  param = { };
> > -	struct nvme_rdma_cm_rep priv = { };
> > +	struct rdma_conn_param  param = {
> > +		.rnr_retry_count = 7,
> > +		.flow_control = 1,
> > +		.initiator_depth = min_t(u8, p->initiator_depth,
> > +			queue->dev->device->attrs.max_qp_init_rd_atom),
> > +		.private_data = &priv,
> > +		.private_data_len = sizeof(priv),
> > +	};
> > +	struct nvme_rdma_cm_rep priv = {
> > +		.recfmt = cpu_to_le16(NVME_RDMA_CM_FMT_1_0),
> > +		.crqsize = cpu_to_le16(queue->recv_queue_size),
> > +	};
> >   	int ret = -ENOMEM;
> >   
> > -	param.rnr_retry_count = 7;
> > -	param.flow_control = 1;
> > -	param.initiator_depth = min_t(u8, p->initiator_depth,
> > -		queue->dev->device->attrs.max_qp_init_rd_atom);
> > -	param.private_data = &priv;
> > -	param.private_data_len = sizeof(priv);
> > -	priv.recfmt = cpu_to_le16(NVME_RDMA_CM_FMT_1_0);
> > -	priv.crqsize = cpu_to_le16(queue->recv_queue_size);
> > -
> >   	ret = rdma_accept(cm_id, &param);
> >   	if (ret)
> >   		pr_err("rdma_accept failed (error code = %d)\n", ret);
> > -- 2.26.2
> 
> What is the issue with existing code that we need this patch for ?
> 

Hello Chaitanya,

This is just a cleanup patch, no functional change intended.

It simply performs the initialization at declaration time, which is how we
usually initialize a subset of all fields.

This is also how it was originally done, but this was changed to a
non-standard way in order to workaround a compiler bug.

Since the compiler bug is no longer relevant, we can go back to the
standard way of doing things.

Performing initialization in a uniform way makes it easier to read and
comprehend the code, especially for people unfamiliar with it, since
it follows the same pattern used in other places of the kernel.

Just reading e.g. struct rdma_conn_param  param = { }; one assumes that
all fields will be zero initialized.. reading futher down in the function
you realize that this function actually does initialize the struct..
which causes a mental hiccup, so you need to do a mental pipeline flush
and go back and read the code from the beginning. This only happens with
patterns that deviate from the standard way of doing things.


Kind regards,
Niklas