On 2024/3/12 15:54, Jan Karcher wrote:
On 11/03/2024 11:59, Wen Gu wrote:
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@xxxxxxxxxx>
---
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@xxxxxxxxxx>
Build-tested-by: Gustavo A. R. Silva <gustavoars@xxxxxxxxxx>
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.
Hi Wen Gu,
you're right. I missed that the offset calculation is broken with your proposal since the full size of the array is
already included in this case which means we would have to subtract the empty slots instead of adding the full ones.
My bad. Thinking about adding a testcase to sxplicit check the size of the CLC Messages send in the future.
That's OK. I am the one who brings this mistake.
Sometimes the details only become clear when start writing the code.
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' ?
Hmm... I think it is way better than priv. One more proposal from my side would be *_fixed since this is the fixed
content and not variable. I'm open for both.
Which one would you prefer more?
'*_fixed' is better, thank you!
Hi Gustavo,
Sorry to complicate things. Could you please post a v2 with the new name updated (avoid using 'hdr') ?
Thank you!
Thanks!
Thanks!
--
Gustavo
[...]
};
Thanks!
Wen Gu
Thanks you
- Jan
