Message-ID: <BYAPR04MB4965AA7B57AC6B6E19BCBBA5869B0@BYAPR04MB4965.namprd04.prod.outlook.com>
Date:   Thu, 18 Jun 2020 17:29:00 +0000
From:   Chaitanya Kulkarni <Chaitanya.Kulkarni@....com>
To:     Niklas Cassel <Niklas.Cassel@....com>
CC:     Christoph Hellwig <hch@....de>, Sagi Grimberg <sagi@...mberg.me>,
        "linux-nvme@...ts.infradead.org" <linux-nvme@...ts.infradead.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 2/2] nvmet: remove workarounds for gcc bug wrt unnamed
 fields in initializers

I'm  not against the code cleanup and it always welcome.
Please also have a look at other comment.

>> 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.
> 
I can see that.
> It simply performs the initialization at declaration time, which is how we
> usually initialize a subset of all fields.
Absolutely not true in case nvme subsystem.
> 
> This is also how it was originally done, but this was changed to a
> non-standard way in order to workaround a compiler bug.
> 
and the existing code matches the pattern present in the repo no need to 
change if it is not causing any problem.
> Since the compiler bug is no longer relevant, we can go back to the
> standard way of doing things.
Is there any problem with the code now which mandates this change ? 
which I don't see any.
> 
> 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.
> 
This is absolutely not uniform way infact what you are proposing is true 
then we need to change all existing function which does not follow this 
pattern, have a look at the following list.

In NVMe subsystem case there are more than 10 functions which are on the 
similar line of this function and doesn't initialize the variable at the 
time declaration.

Please have a look the code :-
nvmf_reg_read32
nvmf_reg_read64
nvmf_reg_write32
nvmf_connect_admin_queue
nvmf_connect_io_queue
nvme_identify_ctrl
nvme_identify_ns_descs
nvme_identify_ns_list
nvme_identify_ns
nvme_features
nvme_get_log
nvme_toggle_streams
nvme_get_stream_params

Also here :-
nvme_user_cmd
nvme_user_cmd64

Last two are an exception of copy_from_user() call before initialization 
case, but we can do copy from user from caller and pass that as an 
argument if we really want to follow the declare-init pattern.

In several places we don't follow this pattern when function is compact 
and it looks ugly for larger structures such as this example. this is
exactly the reason {} used in nvme subsystem.

> Just reading e.g. struct rdma_conn_param  param = { }; one assumes that
> all fields will be zero initialized.. reading futher down in the function

No this is standard style and used in nvme/host/core.c everywhere 
nothing wrong with this at all, please have a look at the author.

> 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.

The function is small enough for such hiccup which follows the pattern 
which we have it in the tree there is nothing wrong.

> 
> Kind regards,
> Niklas
> 

Powered by blists - more mailing lists