Messages in this thread | | | Date | Mon, 11 Mar 2024 18:59:12 +0800 | Subject | Re: [PATCH][next] net/smc: Avoid -Wflex-array-member-not-at-end warnings | From | Wen Gu <> |
| |
On 2024/3/8 07:46, Gustavo A. R. Silva wrote: > > > On 3/7/24 02:17, Jan Karcher wrote: >> >> >> On 04/03/2024 10:00, Wen Gu wrote: >>> >>> >>> On 2024/3/2 02:40, Gustavo A. R. Silva wrote: >>>> -Wflex-array-member-not-at-end is coming in GCC-14, and we are getting >>>> ready to enable it globally. >>>> >>>> There are currently a couple of objects in `struct smc_clc_msg_proposal_area` >>>> that contain a couple of flexible structures: >>>> >> >> Thank you Gustavo for the proposal. >> I had to do some reading to better understand what's happening and how your patch solves this. >> >>>> struct smc_clc_msg_proposal_area { >>>> ... >>>> struct smc_clc_v2_extension pclc_v2_ext; >>>> ... >>>> struct smc_clc_smcd_v2_extension pclc_smcd_v2_ext; >>>> ... >>>> }; >>>> >>>> So, in order to avoid ending up with a couple of flexible-array members >>>> in the middle of a struct, we use the `struct_group_tagged()` helper to >>>> separate the flexible array from the rest of the members in the flexible >>>> structure: >>>> >>>> struct smc_clc_smcd_v2_extension { >>>> struct_group_tagged(smc_clc_smcd_v2_extension_hdr, hdr, >>>> u8 system_eid[SMC_MAX_EID_LEN]; >>>> u8 reserved[16]; >>>> ); >>>> struct smc_clc_smcd_gid_chid gidchid[]; >>>> }; >>>> >>>> With the change described above, we now declare objects of the type of >>>> the tagged struct without embedding flexible arrays in the middle of >>>> another struct: >>>> >>>> struct smc_clc_msg_proposal_area { >>>> ... >>>> struct smc_clc_v2_extension_hdr pclc_v2_ext; >>>> ... >>>> struct smc_clc_smcd_v2_extension_hdr pclc_smcd_v2_ext; >>>> ... >>>> }; >>>> >>>> We also use `container_of()` when we need to retrieve a pointer to the >>>> flexible structures. >>>> >>>> So, with these changes, fix the following warnings: >>>> >>>> In file included from net/smc/af_smc.c:42: >>>> net/smc/smc_clc.h:186:49: warning: structure containing a flexible array member is not at the end of another >>>> structure [-Wflex-array-member-not-at-end] >>>> 186 | struct smc_clc_v2_extension pclc_v2_ext; >>>> | ^~~~~~~~~~~ >>>> net/smc/smc_clc.h:188:49: warning: structure containing a flexible array member is not at the end of another >>>> structure [-Wflex-array-member-not-at-end] >>>> 188 | struct smc_clc_smcd_v2_extension pclc_smcd_v2_ext; >>>> | ^~~~~~~~~~~~~~~~ >>>> >>>> Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org> >>>> --- >>>> net/smc/smc_clc.c | 5 +++-- >>>> net/smc/smc_clc.h | 24 ++++++++++++++---------- >>>> 2 files changed, 17 insertions(+), 12 deletions(-) >>>> >>>> diff --git a/net/smc/smc_clc.c b/net/smc/smc_clc.c >>>> index e55026c7529c..3094cfa1c458 100644 >>>> --- a/net/smc/smc_clc.c >>>> +++ b/net/smc/smc_clc.c >>>> @@ -853,8 +853,9 @@ int smc_clc_send_proposal(struct smc_sock *smc, struct smc_init_info *ini) >>>> pclc_smcd = &pclc->pclc_smcd; >>>> pclc_prfx = &pclc->pclc_prfx; >>>> ipv6_prfx = pclc->pclc_prfx_ipv6; >>>> - v2_ext = &pclc->pclc_v2_ext; >>>> - smcd_v2_ext = &pclc->pclc_smcd_v2_ext; >>>> + v2_ext = container_of(&pclc->pclc_v2_ext, struct smc_clc_v2_extension, _hdr); >>>> + smcd_v2_ext = container_of(&pclc->pclc_smcd_v2_ext, >>>> + struct smc_clc_smcd_v2_extension, hdr); >>>> gidchids = pclc->pclc_gidchids; >>>> trl = &pclc->pclc_trl; >>>> diff --git a/net/smc/smc_clc.h b/net/smc/smc_clc.h >>>> index 7cc7070b9772..5b91a1947078 100644 >>>> --- a/net/smc/smc_clc.h >>>> +++ b/net/smc/smc_clc.h >>>> @@ -134,12 +134,14 @@ struct smc_clc_smcd_gid_chid { >>>> */ >>>> struct smc_clc_v2_extension { >>>> - struct smc_clnt_opts_area_hdr hdr; >>>> - u8 roce[16]; /* RoCEv2 GID */ >>>> - u8 max_conns; >>>> - u8 max_links; >>>> - __be16 feature_mask; >>>> - u8 reserved[12]; >>>> + struct_group_tagged(smc_clc_v2_extension_hdr, _hdr, >>>> + struct smc_clnt_opts_area_hdr hdr; >>>> + u8 roce[16]; /* RoCEv2 GID */ >>>> + u8 max_conns; >>>> + u8 max_links; >>>> + __be16 feature_mask; >>>> + u8 reserved[12]; >>>> + ); >>>> u8 user_eids[][SMC_MAX_EID_LEN]; >>>> }; >>>> @@ -159,8 +161,10 @@ struct smc_clc_msg_smcd { /* SMC-D GID information */ >>>> }; >>>> struct smc_clc_smcd_v2_extension { >>>> - u8 system_eid[SMC_MAX_EID_LEN]; >>>> - u8 reserved[16]; >>>> + struct_group_tagged(smc_clc_smcd_v2_extension_hdr, hdr, >>>> + u8 system_eid[SMC_MAX_EID_LEN]; >>>> + u8 reserved[16]; >>>> + ); >>>> struct smc_clc_smcd_gid_chid gidchid[]; >>>> }; >>>> @@ -183,9 +187,9 @@ struct smc_clc_msg_proposal_area { >>>> struct smc_clc_msg_smcd pclc_smcd; >>>> struct smc_clc_msg_proposal_prefix pclc_prfx; >>>> struct smc_clc_ipv6_prefix pclc_prfx_ipv6[SMC_CLC_MAX_V6_PREFIX]; >>>> - struct smc_clc_v2_extension pclc_v2_ext; >>>> + struct smc_clc_v2_extension_hdr pclc_v2_ext; >>>> u8 user_eids[SMC_CLC_MAX_UEID][SMC_MAX_EID_LEN]; >>>> - struct smc_clc_smcd_v2_extension pclc_smcd_v2_ext; >>>> + struct smc_clc_smcd_v2_extension_hdr pclc_smcd_v2_ext; >>>> struct smc_clc_smcd_gid_chid >>>> pclc_gidchids[SMCD_CLC_MAX_V2_GID_ENTRIES]; >>>> struct smc_clc_msg_trail pclc_trl; >>> >>> Thank you! Gustavo. This patch can fix this warning well, just the name >>> '*_hdr' might not be very accurate, but I don't have a good idea ATM. >> >> I agree. Should we chose this option we should come up for a better name. >> >>> >>> Besides, I am wondering if this can be fixed by moving >>> user_eids of smc_clc_msg_proposal_area into smc_clc_v2_extension, >>> and >>> pclc_gidchids of smc_clc_msg_proposal_area into smc_clc_smcd_v2_extension. >>> >>> so that we can avoid to use the flexible-array in smc_clc_v2_extension >>> and smc_clc_smcd_v2_extension. >> >> I like the idea and put some thought into it. The only thing that is not perfectly clean IMO is the following: >> By the current definition it is easily visible that we are dealing with a variable sized array. If we move them into >> the structs one could think they are always at their MAX size which they are not. >> E.g.: An incoming proposal can have 0 UEIDs indicated by the eid_cnt. >> That said nothing a comment can't fix. >> >> From what i have seen the offset and length calculations regarding the "real" size of those structs is fine with your >> proposal. >> >> Can you verify that your changes also resolve the warnings? > > I can confirm that the changes Wen Gu is proposing also resolve the warnings. > > Wen, > > If you send a proper patch, you can include the following tags: > > Reviewed-by: Gustavo A. R. Silva <gustavoars@kernel.org> > Build-tested-by: Gustavo A. R. Silva <gustavoars@kernel.org> >
Hi Gustavo, thank you for the confirmation that my proposal can fix the warning.
But I found that I may have something missed in my proposal when I think further. My proposal changed the sizes of struct smc_clc_v2_extension and smc_clc_smcd_v2_extension, and some places in SMC need them, such as the fill of kvec in smc_clc_send_proposal().
So my proposal may involve more changes to current SMC code, and I think it is not as clean as your solution. So I perfer yours now.
And as for the name, I think maybe we can use '*_elems' as a suffix, at least it is unambiguous. So it will be smc_clc_v2_extension_elems and smc_clc_smcd_v2_extension_elems.
Jan, what do you think of the name '*_elems' ?
Thanks!
> Thanks! > -- > Gustavo > >> >> [...] >> >>> }; >>> >>> >>> Thanks! >>> Wen Gu >> >> Thanks you >> - Jan
| |