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
>
On Thu, Jun 18, 2020 at 05:29:00PM +0000, Chaitanya Kulkarni wrote:
> 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_features
> nvme_toggle_streams
> nvme_get_stream_params
> nvme_user_cmd
> nvme_user_cmd64
These do not use an initializer at all, these use memset.
So at declaration time, these are not initialized at all.
Basically like:
int a;
a = 2;
I don't have any problem with the memset code pattern per se,
it is very common. Although it is weird that the nvme code
sometimes declares a "struct nvme_command c" on the stack,
and then memsets it, and sometimes uses an initializer.
Perhaps we should create a patch to unify this.
IMHO, using an initializer is more clear.
memset has to be used if the function needs to reset an
existing struct, but in none of the functions above are
we given a nvme_command that we need to reset. In each case
we declare a new nvme_command on the stack (so an initializer
makes more sense).
> nvme_identify_ctrl
> nvme_identify_ns_descs
> nvme_identify_ns_list
> nvme_identify_ns
> nvme_get_log
These used an initializer, and then also later did explict assignments,
e.g.:
struct nvme_command c = { 0 };
...
c.identify.cns = NVME_ID_CNS_CTRL;
which is basically the same as doing:
int a = 0;
a = 2;
However, I have fixed these functions in patch 1/2 in this series,
so that the values are set in the initializer, and then there is
not need for any further assignments.
Basically:
int a = 2;
>
> 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.
If this pattern of using an initializer and then doing an assignment
is the desired pattern, then you should at least remove the
/* gcc-4.4.4 (at least) has issues with initializers and anon unions */
comments from drivers/nvme/host/core.c, because then that comment is
not true, because then you want this design pattern because you like it,
not because of a gcc bug.
>
> > 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.
I think that the standard style in the nvme code is to assign the
values inside the initializer when assigning values to a struct
(except when using memset), see e.g.:
nvmet_tcp_try_recv_ddgst
nvmet_tcp_try_recv_pdu
nvmet_try_send_ddgst
nvmet_rdma_init_srq
nvmet_fc_add_port
nvme_fc_parse_traddr
nvmet_copy_ns_identifier
nvme_tcp_try_send_ddgst
nvme_rdma_inv_rkey
nvme_setup_irqs
nvme_fc_create_ctrl
nvme_fc_parse_traddr
nvme_fc_signal_discovery_scan
nvme_aen_uevent
And then there are the many many
static const struct's that are assigned values using initializers.
(Which I didn't count.)
I made a small mistake in v1, so I'll send out a v2,
feel free to continue the discussion there :)
Kind regards,
Niklas