| Message ID | 1669687459-14180-2-git-send-email-wangyufen@huawei.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v4,1/2] RDMA/hfi1: Fix error return code in parse_platform_config() | expand |
On 11/28/22 18:04, Wang Yufen wrote: > In the previous while loop, "ret" may be assigned zero, , so the error The word "iteration" is missing from the above sentence. Additionally, there is a double comma. > return code may be incorrectly set to 0 instead of -EINVAL. Alse Alse -> Also > case SRP_OPT_QUEUE_SIZE: > - if (match_int(args, &token) || token < 1) { > + ret = match_int(args, &token); > + if (ret) > + goto out; > + if (token < 1) { > pr_warn("bad queue_size parameter '%s'\n", p); > + ret = -EINVAL; > goto out; > } > target->scsi_host->can_queue = token; Why only to report "bad queue_size parameter" if ret == 0 && token < 1 but not if ret < 0? This is a behavior change that has not been explained in the patch description. > @@ -3490,25 +3496,34 @@ static int srp_parse_options(struct net *net, const char *buf, > break; > > case SRP_OPT_MAX_CMD_PER_LUN: > - if (match_int(args, &token) || token < 1) { > + ret = match_int(args, &token); > + if (ret) > + goto out; > + if (token < 1) { > pr_warn("bad max cmd_per_lun parameter '%s'\n", > p); > + ret = -EINVAL; > goto out; > } > target->scsi_host->cmd_per_lun = token; > break; Same comment here and for many other changes below. Thanks, Bart.
I'm so sorry for the poor patch description. Is the following description OK? In the previous iteration of the while loop, "ret" may have been assigned a value of 0, so the error return code -EINVAL may have been incorrectly set to 0. Also, investigate each case separately as Andy suggessted. If the help function match_int() fails, the error code is returned, which is different from the warning information printed before. If the parsing result token is incorrect, "-EINVAL" is returned and the original warning information is printed. Thanks. 在 2022/11/30 2:43, Bart Van Assche 写道: > On 11/28/22 18:04, Wang Yufen wrote: >> In the previous while loop, "ret" may be assigned zero, , so the error > > The word "iteration" is missing from the above sentence. Additionally, > there is a double comma. > >> return code may be incorrectly set to 0 instead of -EINVAL. Alse > > Alse -> Also > >> case SRP_OPT_QUEUE_SIZE: >> - if (match_int(args, &token) || token < 1) { >> + ret = match_int(args, &token); >> + if (ret) >> + goto out; >> + if (token < 1) { >> pr_warn("bad queue_size parameter '%s'\n", p); >> + ret = -EINVAL; >> goto out; >> } >> target->scsi_host->can_queue = token; > > Why only to report "bad queue_size parameter" if ret == 0 && token < 1 > but not if ret < 0? This is a behavior change that has not been > explained in the patch description. > >> @@ -3490,25 +3496,34 @@ static int srp_parse_options(struct net *net, >> const char *buf, >> break; >> case SRP_OPT_MAX_CMD_PER_LUN: >> - if (match_int(args, &token) || token < 1) { >> + ret = match_int(args, &token); >> + if (ret) >> + goto out; >> + if (token < 1) { >> pr_warn("bad max cmd_per_lun parameter '%s'\n", >> p); >> + ret = -EINVAL; >> goto out; >> } >> target->scsi_host->cmd_per_lun = token; >> break; > > Same comment here and for many other changes below. > > Thanks, > > Bart. > >
On 11/29/22 19:31, wangyufen wrote: > I'm so sorry for the poor patch description. Is the following > description OK? > > In the previous iteration of the while loop, "ret" may have been > assigned a value of 0, so the error return code -EINVAL may have been > incorrectly set to 0. > Also, investigate each case separately as Andy suggessted. If the help > function match_int() fails, the error code is returned, which is > different from the warning information printed before. If the parsing > result token is incorrect, "-EINVAL" is returned and the original > warning information is printed. Please reply below instead of above. See also https://en.wikipedia.org/wiki/Posting_style. Regarding your question: not logging an error message if user input is rejected is unfriendly to the user. I think it's better to keep the behavior of reporting an error if a match* function fails instead of reporting in the patch description that the behavior has changed. Thanks, Bart.
在 2022/12/1 2:00, Bart Van Assche 写道: > On 11/29/22 19:31, wangyufen wrote: >> I'm so sorry for the poor patch description. Is the following >> description OK? >> >> In the previous iteration of the while loop, "ret" may have been >> assigned a value of 0, so the error return code -EINVAL may have been >> incorrectly set to 0. >> Also, investigate each case separately as Andy suggessted. If the help >> function match_int() fails, the error code is returned, which is >> different from the warning information printed before. If the parsing >> result token is incorrect, "-EINVAL" is returned and the original >> warning information is printed. > > Please reply below instead of above. See also > https://en.wikipedia.org/wiki/Posting_style. > Thanks, that's helpful. > Regarding your question: not logging an error message if user input is > rejected is unfriendly to the user. I think it's better to keep the > behavior of reporting an error if a match* function fails instead of > reporting in the patch description that the behavior has changed. > > Thanks, > > Bart. > >
在 2022/12/1 2:00, Bart Van Assche 写道: > On 11/29/22 19:31, wangyufen wrote: >> I'm so sorry for the poor patch description. Is the following >> description OK? >> >> In the previous iteration of the while loop, "ret" may have been >> assigned a value of 0, so the error return code -EINVAL may have been >> incorrectly set to 0. >> Also, investigate each case separately as Andy suggessted. If the help >> function match_int() fails, the error code is returned, which is >> different from the warning information printed before. If the parsing >> result token is incorrect, "-EINVAL" is returned and the original >> warning information is printed. > > Please reply below instead of above. See also > https://en.wikipedia.org/wiki/Posting_style. > > Regarding your question: not logging an error message if user input is > rejected is unfriendly to the user. I think it's better to keep the > behavior of reporting an error if a match* function fails instead of > reporting in the patch description that the behavior has changed. > So the following modification is better? case SRP_OPT_CMD_SG_ENTRIES: - if (match_int(args, &token) || token < 1 || token > 255) { + ret = match_int(args, &token); + if (ret) { + pr_warn("bad max cmd_sg_entries parameter '%s'\n", + p); + goto out; + } + if (token < 1 || token > 255) { pr_warn("bad max cmd_sg_entries parameter '%s'\n", p); + ret = -EINVAL; goto out; } target->cmd_sg_cnt = token; break; Or the following is better? if (match_int(args, &token) || token < 1 || token > 255) { pr_warn("bad max cmd_sg_entries parameter '%s'\n", p); + ret = -EINVAL; goto out; } target->cmd_sg_cnt = token; break; > Thanks, > > Bart. > >
On Thu, Dec 01, 2022 at 09:49:51AM +0800, wangyufen wrote: > 在 2022/12/1 2:00, Bart Van Assche 写道: > > On 11/29/22 19:31, wangyufen wrote: ... > case SRP_OPT_CMD_SG_ENTRIES: > - if (match_int(args, &token) || token < 1 || token > > 255) { > + ret = match_int(args, &token); > + if (ret) { > + pr_warn("bad max cmd_sg_entries parameter > '%s'\n", It's misleading message here. The problem is that parser failed by some reason. Otherwise this variant seems good one. > + p); > + goto out; > + } > + if (token < 1 || token > 255) { > pr_warn("bad max cmd_sg_entries parameter > '%s'\n", > p); > + ret = -EINVAL; > goto out; > } > target->cmd_sg_cnt = token; > break; ... > Or the following is better? Why do you want to shadow actual error code?
diff --git a/drivers/infiniband/ulp/srp/ib_srp.c b/drivers/infiniband/ulp/srp/ib_srp.c index 1075c2a..692c69a 100644 --- a/drivers/infiniband/ulp/srp/ib_srp.c +++ b/drivers/infiniband/ulp/srp/ib_srp.c @@ -3343,7 +3343,7 @@ static int srp_parse_options(struct net *net, const char *buf, bool has_port; int opt_mask = 0; int token; - int ret = -EINVAL; + int ret; int i; options = kstrdup(buf, GFP_KERNEL); @@ -3410,7 +3410,8 @@ static int srp_parse_options(struct net *net, const char *buf, break; case SRP_OPT_PKEY: - if (match_hex(args, &token)) { + ret = match_hex(args, &token); + if (ret) { pr_warn("bad P_Key parameter '%s'\n", p); goto out; } @@ -3470,7 +3471,8 @@ static int srp_parse_options(struct net *net, const char *buf, break; case SRP_OPT_MAX_SECT: - if (match_int(args, &token)) { + ret = match_int(args, &token); + if (ret) { pr_warn("bad max sect parameter '%s'\n", p); goto out; } @@ -3478,8 +3480,12 @@ static int srp_parse_options(struct net *net, const char *buf, break; case SRP_OPT_QUEUE_SIZE: - if (match_int(args, &token) || token < 1) { + ret = match_int(args, &token); + if (ret) + goto out; + if (token < 1) { pr_warn("bad queue_size parameter '%s'\n", p); + ret = -EINVAL; goto out; } target->scsi_host->can_queue = token; @@ -3490,25 +3496,34 @@ static int srp_parse_options(struct net *net, const char *buf, break; case SRP_OPT_MAX_CMD_PER_LUN: - if (match_int(args, &token) || token < 1) { + ret = match_int(args, &token); + if (ret) + goto out; + if (token < 1) { pr_warn("bad max cmd_per_lun parameter '%s'\n", p); + ret = -EINVAL; goto out; } target->scsi_host->cmd_per_lun = token; break; case SRP_OPT_TARGET_CAN_QUEUE: - if (match_int(args, &token) || token < 1) { + ret = match_int(args, &token); + if (ret) + goto out; + if (token < 1) { pr_warn("bad max target_can_queue parameter '%s'\n", p); + ret = -EINVAL; goto out; } target->target_can_queue = token; break; case SRP_OPT_IO_CLASS: - if (match_hex(args, &token)) { + ret = match_hex(args, &token); + if (ret) { pr_warn("bad IO class parameter '%s'\n", p); goto out; } @@ -3517,6 +3532,7 @@ static int srp_parse_options(struct net *net, const char *buf, pr_warn("unknown IO class parameter value %x specified (use %x or %x).\n", token, SRP_REV10_IB_IO_CLASS, SRP_REV16A_IB_IO_CLASS); + ret = -EINVAL; goto out; } target->io_class = token; @@ -3539,16 +3555,21 @@ static int srp_parse_options(struct net *net, const char *buf, break; case SRP_OPT_CMD_SG_ENTRIES: - if (match_int(args, &token) || token < 1 || token > 255) { + ret = match_int(args, &token); + if (ret) + goto out; + if (token < 1 || token > 255) { pr_warn("bad max cmd_sg_entries parameter '%s'\n", p); + ret = -EINVAL; goto out; } target->cmd_sg_cnt = token; break; case SRP_OPT_ALLOW_EXT_SG: - if (match_int(args, &token)) { + ret = match_int(args, &token); + if (ret) { pr_warn("bad allow_ext_sg parameter '%s'\n", p); goto out; } @@ -3556,43 +3577,62 @@ static int srp_parse_options(struct net *net, const char *buf, break; case SRP_OPT_SG_TABLESIZE: - if (match_int(args, &token) || token < 1 || - token > SG_MAX_SEGMENTS) { + ret = match_int(args, &token); + if (ret) + goto out; + if (token < 1 || token > SG_MAX_SEGMENTS) { pr_warn("bad max sg_tablesize parameter '%s'\n", p); + ret = -EINVAL; goto out; } target->sg_tablesize = token; break; case SRP_OPT_COMP_VECTOR: - if (match_int(args, &token) || token < 0) { + ret = match_int(args, &token); + if (ret) + goto out; + if (token < 0) { pr_warn("bad comp_vector parameter '%s'\n", p); + ret = -EINVAL; goto out; } target->comp_vector = token; break; case SRP_OPT_TL_RETRY_COUNT: - if (match_int(args, &token) || token < 2 || token > 7) { + ret = match_int(args, &token); + if (ret) + goto out; + if (token < 2 || token > 7) { pr_warn("bad tl_retry_count parameter '%s' (must be a number between 2 and 7)\n", p); + ret = -EINVAL; goto out; } target->tl_retry_count = token; break; case SRP_OPT_MAX_IT_IU_SIZE: - if (match_int(args, &token) || token < 0) { + ret = match_int(args, &token); + if (ret) + goto out; + if (token < 0) { pr_warn("bad maximum initiator to target IU size '%s'\n", p); + ret = -EINVAL; goto out; } target->max_it_iu_size = token; break; case SRP_OPT_CH_COUNT: - if (match_int(args, &token) || token < 1) { + ret = match_int(args, &token); + if (ret) + goto out; + if (token < 1) { pr_warn("bad channel count %s\n", p); + ret = -EINVAL; goto out; } target->ch_count = token; @@ -3601,6 +3641,7 @@ static int srp_parse_options(struct net *net, const char *buf, default: pr_warn("unknown parameter or missing value '%s' in target creation request\n", p); + ret = -EINVAL; goto out; } }
In the previous while loop, "ret" may be assigned zero, , so the error return code may be incorrectly set to 0 instead of -EINVAL. Alse investigate each case separately as Andy suggessted. Fixes: e711f968c49c ("IB/srp: replace custom implementation of hex2bin()") Fixes: 2a174df0c602 ("IB/srp: Use kstrtoull() instead of simple_strtoull()") Fixes: 19f313438c77 ("IB/srp: Add RDMA/CM support") Signed-off-by: Wang Yufen <wangyufen@huawei.com> --- drivers/infiniband/ulp/srp/ib_srp.c | 71 +++++++++++++++++++++++++++++-------- 1 file changed, 56 insertions(+), 15 deletions(-)