On 12.12.23 09:52, Wen Gu wrote:
> The structs of CLC accept and confirm messages for SMCv1 and SMCv2 are
> separately defined and often casted to each other in the code, which may
> increase the risk of errors caused by future divergence of them. So
> unify them into one struct for better maintainability.
>
> Suggested-by: Alexandra Winter <wintera@xxxxxxxxxxxxx>
> Signed-off-by: Wen Gu <guwen@xxxxxxxxxxxxxxxxx>
> ---
> net/smc/af_smc.c | 50 +++++++++++++++---------------------------
> net/smc/smc_clc.c | 65 ++++++++++++++++++++++++-------------------------------
> net/smc/smc_clc.h | 32 ++++++++++-----------------
> 3 files changed, 57 insertions(+), 90 deletions(-)
>
[...]
Thank you very much, Wen Gu. I think this makes it much easier to spot the
places in the accept/confirm code code where v1 vs v2 really make a difference.
I understand that this is not really related to v2.1, but I feel it is worth
simplifying the already complex strucutres before adding even more complexity.
> diff --git a/net/smc/smc_clc.h b/net/smc/smc_clc.h
> index 1697b84..614fa2f 100644
> --- a/net/smc/smc_clc.h
> +++ b/net/smc/smc_clc.h
> @@ -259,29 +259,22 @@ struct smc_clc_fce_gid_ext {
> struct smc_clc_msg_accept_confirm { /* clc accept / confirm message */
> struct smc_clc_msg_hdr hdr;
> union {
> - struct smcr_clc_msg_accept_confirm r0; /* SMC-R */
> - struct { /* SMC-D */
> - struct smcd_clc_msg_accept_confirm_common d0;
> - u32 reserved5[3];
> - };
> - };
> -} __packed; /* format defined in RFC7609 */
> -
> -struct smc_clc_msg_accept_confirm_v2 { /* clc accept / confirm message */
> - struct smc_clc_msg_hdr hdr;
> - union {
> struct { /* SMC-R */
> - struct smcr_clc_msg_accept_confirm r0;
> + struct smcr_clc_msg_accept_confirm _r0;
> + /* v2 only, reserved and ignored in v1: */
> u8 eid[SMC_MAX_EID_LEN];
> u8 reserved6[8];
> } r1;
> struct { /* SMC-D */
> - struct smcd_clc_msg_accept_confirm_common d0;
> + struct smcd_clc_msg_accept_confirm_common _d0;
> + /* v2 only, reserved and ignored in v1: */
> __be16 chid;
> u8 eid[SMC_MAX_EID_LEN];
> u8 reserved5[8];
> } d1;
> };
> +#define r0 r1._r0
> +#define d0 d1._d0
This adds complexity.
If you add the v2-only fields to struct smcr_clc_msg_accept_confirm and
struct smcd_clc_msg_accept_confirm_common respectively, you can avoid the
#define and the extra layer in the struct.
Actually there are already v2-only fields in smcd_clc_msg_accept_confirm_common
and smcd_clc_msg_accept_confirm_common (gid and others). So you could add the
correct informative comments there.
> };
You have removed the __packed attribute.
patch 07/10 adds it back for the SMC-D case, but the SMC-R case needs it as well.
[...]
