Messages in this thread | | | From | Chaitanya Kulkarni <> | Subject | Re: [PATCH 2/2] nvmet: remove workarounds for gcc bug wrt unnamed fields in initializers | Date | Thu, 18 Jun 2020 17:29:00 +0000 |
| |
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 >
| |