-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:
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 <[email protected]>
---
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;
--
2.34.1
On Fri, Mar 01, 2024 at 12:40:57PM -0600, 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:
>
> 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 <[email protected]>
I think this is a nice way to deal with these flex-array cases. Using
the struct_group() and container_of() means there is very little
collateral impact. Since this is isolated to a single file, I wonder if
it's easy to check that there are no binary differences too? I wouldn't
expect any -- container_of() is all constant expressions, so the
assignment offsets should all be the same, etc.
Reviewed-by: Kees Cook <[email protected]>
-Kees
--
Kees Cook
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:
>
> 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 <[email protected]>
> ---
> 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.
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.
diff --git a/net/smc/smc_clc.c b/net/smc/smc_clc.c
index e55026c7529c..971b4677b84d 100644
--- a/net/smc/smc_clc.c
+++ b/net/smc/smc_clc.c
@@ -855,7 +855,7 @@ int smc_clc_send_proposal(struct smc_sock *smc, struct smc_init_info *ini)
ipv6_prfx = pclc->pclc_prfx_ipv6;
v2_ext = &pclc->pclc_v2_ext;
smcd_v2_ext = &pclc->pclc_smcd_v2_ext;
- gidchids = pclc->pclc_gidchids;
+ gidchids = pclc->pclc_smcd_v2_ext.gidchid;
trl = &pclc->pclc_trl;
pclc_base->hdr.version = SMC_V2;
diff --git a/net/smc/smc_clc.h b/net/smc/smc_clc.h
index 7cc7070b9772..36c8432af73e 100644
--- a/net/smc/smc_clc.h
+++ b/net/smc/smc_clc.h
@@ -133,6 +133,14 @@ struct smc_clc_smcd_gid_chid {
* (https://www.ibm.com/support/pages/node/6326337)
*/
+#define SMC_CLC_MAX_V6_PREFIX 8
+#define SMC_CLC_MAX_UEID 8
+#define SMCD_CLC_MAX_V2_GID_ENTRIES 8 /* max # of CHID-GID entries in CLC
+ * proposal SMC-Dv2 extension.
+ * each ISM device takes one entry and
+ * each Emulated-ISM takes two entries
+ */
+
struct smc_clc_v2_extension {
struct smc_clnt_opts_area_hdr hdr;
u8 roce[16]; /* RoCEv2 GID */
@@ -140,7 +148,7 @@ struct smc_clc_v2_extension {
u8 max_links;
__be16 feature_mask;
u8 reserved[12];
- u8 user_eids[][SMC_MAX_EID_LEN];
+ u8 user_eids[SMC_CLC_MAX_UEID][SMC_MAX_EID_LEN];
};
struct smc_clc_msg_proposal_prefix { /* prefix part of clc proposal message*/
@@ -161,7 +169,7 @@ 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 smc_clc_smcd_gid_chid gidchid[];
+ struct smc_clc_smcd_gid_chid gidchid[SMCD_CLC_MAX_V2_GID_ENTRIES];
};
struct smc_clc_msg_proposal { /* clc proposal message sent by Linux */
@@ -170,24 +178,13 @@ struct smc_clc_msg_proposal { /* clc proposal message sent by Linux */
__be16 iparea_offset; /* offset to IP address information area */
} __aligned(4);
-#define SMC_CLC_MAX_V6_PREFIX 8
-#define SMC_CLC_MAX_UEID 8
-#define SMCD_CLC_MAX_V2_GID_ENTRIES 8 /* max # of CHID-GID entries in CLC
- * proposal SMC-Dv2 extension.
- * each ISM device takes one entry and
- * each Emulated-ISM takes two entries
- */
-
struct smc_clc_msg_proposal_area {
struct smc_clc_msg_proposal pclc_base;
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;
- 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_gid_chid
- pclc_gidchids[SMCD_CLC_MAX_V2_GID_ENTRIES];
struct smc_clc_msg_trail pclc_trl;
};
Thanks!
Wen Gu
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 <[email protected]>
>> ---
>> 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?
[...]
> };
>
>
> Thanks!
> Wen Gu
Thanks you
- Jan
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 <[email protected]>
>>> ---
>>> 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 <[email protected]>
Build-tested-by: Gustavo A. R. Silva <[email protected]>
Thanks!
--
Gustavo
>
> [...]
>
>> };
>>
>>
>> Thanks!
>> Wen Gu
>
> Thanks you
> - Jan
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 <[email protected]>
>>>> ---
>>>> 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 <[email protected]>
> Build-tested-by: Gustavo A. R. Silva <[email protected]>
>
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
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 <[email protected]>
>>>>> ---
>>>>> 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 <[email protected]>
>> Build-tested-by: Gustavo A. R. Silva <[email protected]>
>>
>
> 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.
>
> 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?
>
> Thanks!
>
>> Thanks!
>> --
>> Gustavo
>>
>>>
>>> [...]
>>>
>>>> };
>>>>
>>>>
>>>> Thanks!
>>>> Wen Gu
>>>
>>> Thanks you
>>> - Jan
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 <[email protected]>
>>>>>> ---
>>>>>> 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 <[email protected]>
>>> Build-tested-by: Gustavo A. R. Silva <[email protected]>
>>>
>>
>> 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