<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=us-ascii">
</head>
<body>
<br>
Huh. I see your point, and the logic of it is obviously sound, but I’m curious - I don’t think I’ve ever seen names like those you gave used. I’ve only seen MAX.<br>
<br>
Am I wrong about that, or is MAX indeed the dominant choice?<br>
<br>
I also think in context, used in caps, MAX is clearly a special value without other meaning.<br>
<hr style="display:inline-block;width:98%" tabindex="-1">
<div id="divRplyFwdMsg" dir="ltr"><font face="Calibri, sans-serif" style="font-size:11pt" color="#000000"><b>From:</b> NeilBrown <neilb@suse.com><br>
<b>Sent:</b> Tuesday, July 31, 2018 7:23:21 PM<br>
<b>To:</b> Patrick Farrell; James Simmons; Andreas Dilger; Oleg Drokin<br>
<b>Cc:</b> Lustre Development List<br>
<b>Subject:</b> Re: [lustre-devel] [PATCH 25/31] lustre: config: move config types into lustre_idl.h</font>
<div> </div>
</div>
<div class="BodyFragment"><font size="2"><span style="font-size:11pt;">
<div class="PlainText">On Tue, Jul 31 2018, Patrick Farrell wrote:<br>
<br>
> Neil,<br>
><br>
> Do you have an objection to the concept, or just because this one's not used?<br>
> Having a MAX makes it easy to write things like < MYENUM_MAX as sanity checking code, and then if the enum is added to, it still works. Seems useful to me.<br>
<br>
I object to the name. "MAX" is short for "MAXIMUM" which means the<br>
highest value that is actually used. When comparing something to the<br>
maximum it makes sense to say<br>
if (foo <= maximum)<br>
<br>
but it rarely makes sense to say<br>
if (foo < maximum)<br>
<br>
If you want a count of the number of values, use MYENUM_CNT or<br>
MYENUM_NUM. This can sensibly be one more than the maximum value.<br>
But if you have MYSENUM_MAX, make sure it is the maximum meaningful<br>
value for the enum.<br>
</rant><br>
<br>
Thanks,<br>
NeilBrown<br>
<br>
<br>
><br>
> - Patrick<br>
> ________________________________<br>
> From: lustre-devel <lustre-devel-bounces@lists.lustre.org> on behalf of NeilBrown <neilb@suse.com><br>
> Sent: Tuesday, July 31, 2018 5:47:28 PM<br>
> To: James Simmons; Andreas Dilger; Oleg Drokin<br>
> Cc: Lustre Development List<br>
> Subject: Re: [lustre-devel] [PATCH 25/31] lustre: config: move config types into lustre_idl.h<br>
><br>
> On Mon, Jul 30 2018, James Simmons wrote:<br>
><br>
>> From: Niu Yawei <yawei.niu@intel.com><br>
>><br>
>> Move config type values CONFIG_T_XXX into lustre_idl.h since they<br>
>> will be put on wire when reading config logs.<br>
>><br>
>> Add missing wire checks for mgs_nidtbl_entry, mgs_config_body and<br>
>> mgs_config_res.<br>
>><br>
>> Redefine CONFIG_SUB_XXX for the sub clds attached on config log.<br>
>><br>
>> Signed-off-by: Niu Yawei <yawei.niu@intel.com><br>
>> WC-id: <a href="https://jira.whamcloud.com/browse/LU-9216">https://jira.whamcloud.com/browse/LU-9216</a><br>
>> Reviewed-on: <a href="https://review.whamcloud.com/26022">https://review.whamcloud.com/26022</a><br>
>> Reviewed-by: Fan Yong <fan.yong@intel.com><br>
>> Reviewed-by: John L. Hammond <jhammond@whamcloud.com><br>
>> Reviewed-by: Oleg Drokin <green@whamcloud.com><br>
>> Signed-off-by: James Simmons <jsimmons@infradead.org><br>
>> ---<br>
>> .../lustre/include/uapi/linux/lustre/lustre_idl.h | 10 ++-<br>
>> drivers/staging/lustre/lustre/include/obd_class.h | 12 +--<br>
>> drivers/staging/lustre/lustre/mgc/mgc_request.c | 6 +-<br>
>> drivers/staging/lustre/lustre/ptlrpc/wiretest.c | 85 ++++++++++++++++++++++<br>
>> 4 files changed, 103 insertions(+), 10 deletions(-)<br>
>><br>
>> diff --git a/drivers/staging/lustre/include/uapi/linux/lustre/lustre_idl.h b/drivers/staging/lustre/include/uapi/linux/lustre/lustre_idl.h<br>
>> index c9b32ef..bd3b45a 100644<br>
>> --- a/drivers/staging/lustre/include/uapi/linux/lustre/lustre_idl.h<br>
>> +++ b/drivers/staging/lustre/include/uapi/linux/lustre/lustre_idl.h<br>
>> @@ -2111,11 +2111,19 @@ struct mgs_nidtbl_entry {<br>
>> } u;<br>
>> };<br>
>><br>
>> +enum {<br>
>> + CONFIG_T_CONFIG = 0,<br>
>> + CONFIG_T_SPTLRPC = 1,<br>
>> + CONFIG_T_RECOVER = 2,<br>
>> + CONFIG_T_PARAMS = 3,<br>
>> + CONFIG_T_MAX<br>
><br>
> Arrrgggh. It's back. I thought we had killed CONFIG_T_MAX (which isn't<br>
> a MAX).<br>
> It's never used, so it'll have to go.<br>
><br>
> NeilBrown<br>
><br>
><br>
>> +};<br>
>> +<br>
>> struct mgs_config_body {<br>
>> char mcb_name[MTI_NAME_MAXLEN]; /* logname */<br>
>> __u64 mcb_offset; /* next index of config log to request */<br>
>> __u16 mcb_type; /* type of log: CONFIG_T_[CONFIG|RECOVER] */<br>
>> - __u8 mcb_reserved;<br>
>> + __u8 mcb_nm_cur_pass;<br>
>> __u8 mcb_bits; /* bits unit size of config log */<br>
>> __u32 mcb_units; /* # of units for bulk transfer */<br>
>> };<br>
>> diff --git a/drivers/staging/lustre/lustre/include/obd_class.h b/drivers/staging/lustre/lustre/include/obd_class.h<br>
>> index 184da99..647cc22 100644<br>
>> --- a/drivers/staging/lustre/lustre/include/obd_class.h<br>
>> +++ b/drivers/staging/lustre/lustre/include/obd_class.h<br>
>> @@ -156,16 +156,16 @@ struct config_llog_instance {<br>
>> int class_config_parse_llog(const struct lu_env *env, struct llog_ctxt *ctxt,<br>
>> char *name, struct config_llog_instance *cfg);<br>
>><br>
>> -#define CONFIG_T_CONFIG BIT(0)<br>
>> -#define CONFIG_T_SPTLRPC BIT(1)<br>
>> -#define CONFIG_T_RECOVER BIT(2)<br>
>> -#define CONFIG_T_PARAMS BIT(3)<br>
>> +#define CONFIG_SUB_CONFIG BIT(0)<br>
>> +#define CONFIG_SUB_SPTLRPC BIT(1)<br>
>> +#define CONFIG_SUB_RECOVER BIT(2)<br>
>> +#define CONFIG_SUB_PARAMS BIT(3)<br>
>><br>
>> /* Sub clds should be attached to the config_llog_data when processing<br>
>> * config log for client or server target.<br>
>> */<br>
>> -#define CONFIG_SUB_CLIENT (CONFIG_T_SPTLRPC | CONFIG_T_RECOVER | \<br>
>> - CONFIG_T_PARAMS)<br>
>> +#define CONFIG_SUB_CLIENT (CONFIG_SUB_SPTLRPC | CONFIG_SUB_RECOVER | \<br>
>> + CONFIG_SUB_PARAMS)<br>
>><br>
>> #define PARAMS_FILENAME "params"<br>
>> #define LCTL_UPCALL "lctl"<br>
>> diff --git a/drivers/staging/lustre/lustre/mgc/mgc_request.c b/drivers/staging/lustre/lustre/mgc/mgc_request.c<br>
>> index 06fcc7e..833e6a0 100644<br>
>> --- a/drivers/staging/lustre/lustre/mgc/mgc_request.c<br>
>> +++ b/drivers/staging/lustre/lustre/mgc/mgc_request.c<br>
>> @@ -315,7 +315,7 @@ struct config_llog_data *do_config_log_add(struct obd_device *obd,<br>
>> memcpy(seclogname, logname, ptr - logname);<br>
>> strcpy(seclogname + (ptr - logname), "-sptlrpc");<br>
>><br>
>> - if (cfg->cfg_sub_clds & CONFIG_T_SPTLRPC) {<br>
>> + if (cfg->cfg_sub_clds & CONFIG_SUB_SPTLRPC) {<br>
>> sptlrpc_cld = config_log_find_or_add(obd, seclogname, NULL,<br>
>> CONFIG_T_SPTLRPC, cfg);<br>
>> if (IS_ERR(sptlrpc_cld)) {<br>
>> @@ -325,7 +325,7 @@ struct config_llog_data *do_config_log_add(struct obd_device *obd,<br>
>> }<br>
>> }<br>
>><br>
>> - if (cfg->cfg_sub_clds & CONFIG_T_PARAMS) {<br>
>> + if (cfg->cfg_sub_clds & CONFIG_SUB_PARAMS) {<br>
>> params_cld = config_log_find_or_add(obd, PARAMS_FILENAME, sb,<br>
>> CONFIG_T_PARAMS, cfg);<br>
>> if (IS_ERR(params_cld)) {<br>
>> @@ -345,7 +345,7 @@ struct config_llog_data *do_config_log_add(struct obd_device *obd,<br>
>><br>
>> LASSERT(lsi->lsi_lmd);<br>
>> if (!(lsi->lsi_lmd->lmd_flags & LMD_FLG_NOIR) &&<br>
>> - cfg->cfg_sub_clds & CONFIG_T_RECOVER) {<br>
>> + cfg->cfg_sub_clds & CONFIG_SUB_RECOVER) {<br>
>> ptr = strrchr(seclogname, '-');<br>
>> if (ptr) {<br>
>> *ptr = 0;<br>
>> diff --git a/drivers/staging/lustre/lustre/ptlrpc/wiretest.c b/drivers/staging/lustre/lustre/ptlrpc/wiretest.c<br>
>> index 2f081ed..09b1298 100644<br>
>> --- a/drivers/staging/lustre/lustre/ptlrpc/wiretest.c<br>
>> +++ b/drivers/staging/lustre/lustre/ptlrpc/wiretest.c<br>
>> @@ -3629,6 +3629,91 @@ void lustre_assert_wire_constants(void)<br>
>> LASSERTF((int)sizeof(((struct mgs_target_info *)0)->mti_params) == 4096, "found %lld\n",<br>
>> (long long)(int)sizeof(((struct mgs_target_info *)0)->mti_params));<br>
>><br>
>> + /* Checks for struct mgs_nidtbl_entry */<br>
>> + LASSERTF((int)sizeof(struct mgs_nidtbl_entry) == 24, "found %lld\n",<br>
>> + (long long)(int)sizeof(struct mgs_nidtbl_entry));<br>
>> + LASSERTF((int)offsetof(struct mgs_nidtbl_entry, mne_version) == 0, "found %lld\n",<br>
>> + (long long)(int)offsetof(struct mgs_nidtbl_entry, mne_version));<br>
>> + LASSERTF((int)sizeof(((struct mgs_nidtbl_entry *)0)->mne_version) == 8, "found %lld\n",<br>
>> + (long long)(int)sizeof(((struct mgs_nidtbl_entry *)0)->mne_version));<br>
>> + LASSERTF((int)offsetof(struct mgs_nidtbl_entry, mne_instance) == 8, "found %lld\n",<br>
>> + (long long)(int)offsetof(struct mgs_nidtbl_entry, mne_instance));<br>
>> + LASSERTF((int)sizeof(((struct mgs_nidtbl_entry *)0)->mne_instance) == 4, "found %lld\n",<br>
>> + (long long)(int)sizeof(((struct mgs_nidtbl_entry *)0)->mne_instance));<br>
>> + LASSERTF((int)offsetof(struct mgs_nidtbl_entry, mne_index) == 12, "found %lld\n",<br>
>> + (long long)(int)offsetof(struct mgs_nidtbl_entry, mne_index));<br>
>> + LASSERTF((int)sizeof(((struct mgs_nidtbl_entry *)0)->mne_index) == 4, "found %lld\n",<br>
>> + (long long)(int)sizeof(((struct mgs_nidtbl_entry *)0)->mne_index));<br>
>> + LASSERTF((int)offsetof(struct mgs_nidtbl_entry, mne_length) == 16, "found %lld\n",<br>
>> + (long long)(int)offsetof(struct mgs_nidtbl_entry, mne_length));<br>
>> + LASSERTF((int)sizeof(((struct mgs_nidtbl_entry *)0)->mne_length) == 4, "found %lld\n",<br>
>> + (long long)(int)sizeof(((struct mgs_nidtbl_entry *)0)->mne_length));<br>
>> + LASSERTF((int)offsetof(struct mgs_nidtbl_entry, mne_type) == 20, "found %lld\n",<br>
>> + (long long)(int)offsetof(struct mgs_nidtbl_entry, mne_type));<br>
>> + LASSERTF((int)sizeof(((struct mgs_nidtbl_entry *)0)->mne_type) == 1, "found %lld\n",<br>
>> + (long long)(int)sizeof(((struct mgs_nidtbl_entry *)0)->mne_type));<br>
>> + LASSERTF((int)offsetof(struct mgs_nidtbl_entry, mne_nid_type) == 21, "found %lld\n",<br>
>> + (long long)(int)offsetof(struct mgs_nidtbl_entry, mne_nid_type));<br>
>> + LASSERTF((int)sizeof(((struct mgs_nidtbl_entry *)0)->mne_nid_type) == 1, "found %lld\n",<br>
>> + (long long)(int)sizeof(((struct mgs_nidtbl_entry *)0)->mne_nid_type));<br>
>> + LASSERTF((int)offsetof(struct mgs_nidtbl_entry, mne_nid_size) == 22, "found %lld\n",<br>
>> + (long long)(int)offsetof(struct mgs_nidtbl_entry, mne_nid_size));<br>
>> + LASSERTF((int)sizeof(((struct mgs_nidtbl_entry *)0)->mne_nid_size) == 1, "found %lld\n",<br>
>> + (long long)(int)sizeof(((struct mgs_nidtbl_entry *)0)->mne_nid_size));<br>
>> + LASSERTF((int)offsetof(struct mgs_nidtbl_entry, mne_nid_count) == 23, "found %lld\n",<br>
>> + (long long)(int)offsetof(struct mgs_nidtbl_entry, mne_nid_count));<br>
>> + LASSERTF((int)sizeof(((struct mgs_nidtbl_entry *)0)->mne_nid_count) == 1, "found %lld\n",<br>
>> + (long long)(int)sizeof(((struct mgs_nidtbl_entry *)0)->mne_nid_count));<br>
>> + LASSERTF((int)offsetof(struct mgs_nidtbl_entry, u.nids[0]) == 24, "found %lld\n",<br>
>> + (long long)(int)offsetof(struct mgs_nidtbl_entry, u.nids[0]));<br>
>> + LASSERTF((int)sizeof(((struct mgs_nidtbl_entry *)0)->u.nids[0]) == 8, "found %lld\n",<br>
>> + (long long)(int)sizeof(((struct mgs_nidtbl_entry *)0)->u.nids[0]));<br>
>> +<br>
>> + /* Checks for struct mgs_config_body */<br>
>> + LASSERTF((int)sizeof(struct mgs_config_body) == 80, "found %lld\n",<br>
>> + (long long)(int)sizeof(struct mgs_config_body));<br>
>> + LASSERTF((int)offsetof(struct mgs_config_body, mcb_name) == 0, "found %lld\n",<br>
>> + (long long)(int)offsetof(struct mgs_config_body, mcb_name));<br>
>> + LASSERTF((int)sizeof(((struct mgs_config_body *)0)->mcb_name) == 64, "found %lld\n",<br>
>> + (long long)(int)sizeof(((struct mgs_config_body *)0)->mcb_name));<br>
>> + LASSERTF((int)offsetof(struct mgs_config_body, mcb_offset) == 64, "found %lld\n",<br>
>> + (long long)(int)offsetof(struct mgs_config_body, mcb_offset));<br>
>> + LASSERTF((int)sizeof(((struct mgs_config_body *)0)->mcb_offset) == 8, "found %lld\n",<br>
>> + (long long)(int)sizeof(((struct mgs_config_body *)0)->mcb_offset));<br>
>> + LASSERTF((int)offsetof(struct mgs_config_body, mcb_type) == 72, "found %lld\n",<br>
>> + (long long)(int)offsetof(struct mgs_config_body, mcb_type));<br>
>> + LASSERTF((int)sizeof(((struct mgs_config_body *)0)->mcb_type) == 2, "found %lld\n",<br>
>> + (long long)(int)sizeof(((struct mgs_config_body *)0)->mcb_type));<br>
>> + LASSERTF((int)offsetof(struct mgs_config_body, mcb_nm_cur_pass) == 74, "found %lld\n",<br>
>> + (long long)(int)offsetof(struct mgs_config_body, mcb_nm_cur_pass));<br>
>> + LASSERTF((int)sizeof(((struct mgs_config_body *)0)->mcb_nm_cur_pass) == 1, "found %lld\n",<br>
>> + (long long)(int)sizeof(((struct mgs_config_body *)0)->mcb_nm_cur_pass));<br>
>> + LASSERTF((int)offsetof(struct mgs_config_body, mcb_bits) == 75, "found %lld\n",<br>
>> + (long long)(int)offsetof(struct mgs_config_body, mcb_bits));<br>
>> + LASSERTF((int)sizeof(((struct mgs_config_body *)0)->mcb_bits) == 1, "found %lld\n",<br>
>> + (long long)(int)sizeof(((struct mgs_config_body *)0)->mcb_bits));<br>
>> + LASSERTF((int)offsetof(struct mgs_config_body, mcb_units) == 76, "found %lld\n",<br>
>> + (long long)(int)offsetof(struct mgs_config_body, mcb_units));<br>
>> + LASSERTF((int)sizeof(((struct mgs_config_body *)0)->mcb_units) == 4, "found %lld\n",<br>
>> + (long long)(int)sizeof(((struct mgs_config_body *)0)->mcb_units));<br>
>> +<br>
>> + BUILD_BUG_ON(CONFIG_T_CONFIG != 0);<br>
>> + BUILD_BUG_ON(CONFIG_T_SPTLRPC != 1);<br>
>> + BUILD_BUG_ON(CONFIG_T_RECOVER != 2);<br>
>> + BUILD_BUG_ON(CONFIG_T_PARAMS != 3);<br>
>> +<br>
>> + /* Checks for struct mgs_config_res */<br>
>> + LASSERTF((int)sizeof(struct mgs_config_res) == 16, "found %lld\n",<br>
>> + (long long)(int)sizeof(struct mgs_config_res));<br>
>> + LASSERTF((int)offsetof(struct mgs_config_res, mcr_offset) == 0, "found %lld\n",<br>
>> + (long long)(int)offsetof(struct mgs_config_res, mcr_offset));<br>
>> + LASSERTF((int)sizeof(((struct mgs_config_res *)0)->mcr_offset) == 8, "found %lld\n",<br>
>> + (long long)(int)sizeof(((struct mgs_config_res *)0)->mcr_offset));<br>
>> + LASSERTF((int)offsetof(struct mgs_config_res, mcr_size) == 8, "found %lld\n",<br>
>> + (long long)(int)offsetof(struct mgs_config_res, mcr_size));<br>
>> + LASSERTF((int)sizeof(((struct mgs_config_res *)0)->mcr_size) == 8, "found %lld\n",<br>
>> + (long long)(int)sizeof(((struct mgs_config_res *)0)->mcr_size));<br>
>> +<br>
>> /* Checks for struct lustre_capa */<br>
>> LASSERTF((int)sizeof(struct lustre_capa) == 120, "found %lld\n",<br>
>> (long long)(int)sizeof(struct lustre_capa));<br>
>> --<br>
>> 1.8.3.1<br>
</div>
</span></font></div>
</body>
</html>