Messages in this thread | | | Date | Thu, 17 Aug 2023 11:23:09 +0800 | Subject | Re: [PATCH net-next 2/6] net/smc: add vendor unique experimental options area in clc handshake | From | Guangguan Wang <> |
| |
On 2023/8/17 05:49, Jan Karcher wrote: > Hi Guangguan Wang, > > thank you, some minor thoughts on this one. > > On 16/08/2023 10:33, Guangguan Wang wrote: ... >> -static void smc_clc_fill_fce(struct smc_clc_first_contact_ext *fce, int *len, int release_nr) >> +static int smc_clc_fill_fce(struct smc_clc_first_contact_ext_v2x *fce, >> + struct smc_init_info *ini) >> { >> + int ret = sizeof(*fce); >> + >> memset(fce, 0, sizeof(*fce)); >> - fce->os_type = SMC_CLC_OS_LINUX; >> - fce->release = release_nr; >> - memcpy(fce->hostname, smc_hostname, sizeof(smc_hostname)); >> - (*len) += sizeof(*fce); >> + fce->fce_v20.os_type = SMC_CLC_OS_LINUX; >> + fce->fce_v20.release = ini->release_nr; > > I don't like that this is called fce_v20.release which can be set to v2.1 here although the struct is named v20. Maybe let us call the struct something like fce_v2_base or fce_base_v2. >
fce_v2_base sounds better.
>> diff --git a/net/smc/smc_clc.h b/net/smc/smc_clc.h >> index b923e89acafb..6133276a8839 100644 >> --- a/net/smc/smc_clc.h >> +++ b/net/smc/smc_clc.h >> @@ -147,7 +147,9 @@ struct smc_clc_msg_proposal_prefix { /* prefix part of clc proposal message*/ >> struct smc_clc_msg_smcd { /* SMC-D GID information */ >> struct smc_clc_smcd_gid_chid ism; /* ISM native GID+CHID of requestor */ >> __be16 v2_ext_offset; /* SMC Version 2 Extension Offset */ >> - u8 reserved[28]; >> + u8 vendor_oui[3]; >> + u8 vendor_exp_options[5]; >> + u8 reserved[20]; > > Could we either make those variables a bit more self explaining via their name (e.g. vendor_organization_uid) or adding a comment /* vendor organizationally unique identifier */ >
I will fix it in the next version.
>> }; >> struct smc_clc_smcd_v2_extension { >> @@ -231,8 +233,17 @@ struct smc_clc_first_contact_ext { >> u8 hostname[SMC_MAX_HOSTNAME_LEN]; >> }; >> +struct smc_clc_first_contact_ext_v2x { >> + struct smc_clc_first_contact_ext fce_v20; > > as stated at the top where the release is assigned i'm not completly happy with the naming. >
Thanks, Guangguan Wang
| |