Re: [PATCH net-next v6 03/10] net/smc: unify the structs of accept or confirm message for v1 and v2

[Alexandra Winter]

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.


[...]