Re: [PATCH net-next v5 2/9] net/smc: introduce sub-functions for smc_clc_send_confirm_accept()

[Wen Gu]

On 2023/12/11 21:35, Alexandra Winter wrote:
> On 11.12.23 13:15, Wen Gu wrote:
> > > > +    clc = (struct smc_clc_msg_accept_confirm *)clc_v2;
> > >
> > > Why is this cast neccessary? (Here as well as in smcr_clc_prep_confirm_accept
> > > and in smc_clc_send_confirm_accept)
> > > smc_clc_msg_accept_confirm_v2 has hdr and d0 as well.
> >
> > I think the cast is to imply that v2 is an expansion of v1, or v1 is the base of v2.
> > So here using clc(v1) reperesents their common set.
> >
> > If we use smc_clc_msg_accept_confirm_v2 for all, I think readers may be tempted to
> > check whether the hdr and d0 in 'smc_clc_msg_accept_confirm_v2' are also applicable to v1.
> >
> > And there are settings below that are specific for v1. It may be confusing if we
> > change it like this:
> >
> > if (version == SMC_V1) {
> >      clc_v2->hdr.length = htons(SMCD_CLC_ACCEPT_CONFIRM_LEN);
> > } else {
> >
> >
> > > IMO, it would be a nice seperate patch to get rid of the 2 type defs for
> > > smc_clc_msg_accept_confirm and smc_clc_msg_accept_confirm_v2
> > > and all the related casting anyhow.
> >
> > Do you mean to define only smc_clc_msg_accept_confirm_v2 or define with the name
> > of smc_clc_msg_accept_confirm but the contents of smc_clc_msg_accept_confirm_v2?
> >
> > I have a different opinion on this, since I think the smc_clc_msg_accept_confirm
> > and smc_clc_msg_accept_confirm_v2 clearly shows the difference between v1 and
> > v2 messages and remind people what is currently working on. So I perfer to keep them.
> > Am I missing something?
>
>
> This is a discussion about coding style, readability and maintainability (avoid future errors).
> And the code works today and the rest is opinions. That said, let me list some arguments why
> I don't like the casts.
>
> Casts in general break the type checking of the compiler.
>
> In some places e.g. clc.d0 points to struct smc_clc_msg_accept_confirm in other
> places it points to struct smc_clc_msg_accept_confirm_v2.
> This makes it hard to find all places where e.g. d0 is altered. (e.g. with an IDE).
>
> You say: "smc_clc_msg_accept_confirm
> > and smc_clc_msg_accept_confirm_v2 clearly shows the difference between v1 and
> > v2 messages"
> But that is not even the case in the code that this patch changes:
> In smcd_clc_prep_confirm_accept() you pass a struct smc_clc_msg_accept_confirm_v2
> cast it to v1 (even in the v2 case) and then use the v1 layout for the common fields and
> the v1-only fields. So I don't think that helps very much.
>
> The v2 messages were explicitely defined for compatibility. i.e.
> all v1 fields are still available. It would be good to see that in the code as well.
> With 2 differnet structs you don't emphasize that.
>
> With future changes somebody could easily make a mistake that the 2 structures don't
> have the same size anymore. And then the casting can lead to out-of-bound error that
> are hard to find.
>
> We want v2 to be the usual case and v1 to be the exception for backwards compatibility.
> FOr historic reasons, the code looks as if v2 is the exception. I'd rather point out the
> remaining v1 cases.
>
>
>
> I could envision something like:
>
> struct smc_clc_msg_accept_confirm {	/* clc accept / confirm message */
> 	struct smc_clc_msg_hdr hdr;
> 	union {
> 		struct { /* SMC-R */
> 			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;
> 			/* v2 only, reserved and ignored in v1: */
> 			__be16 chid;
> 			u8 eid[SMC_MAX_EID_LEN];
> 			__be64 gid_ext;
> 		} __packed d1;
> 	};
> };
>
> And then only use this one structure.

Thank you Sandy for the detailed explanation.

What I considered, as mentioned above, is that if the two are combined,
it may be difficult to distinguish according to the name what situation
I am in, v1 or v2?

But I do agree with your concern about the potential errors that caused
by future divergence of the two struct if they are defined separately.

I will try to combine them into one struct in a seperate patch.

Thank you.