Hello,
On Wed, 15 Nov 2017 20:47:14 +0530
Hari Bathini <[email protected]> wrote:
> From: Michal Suchanek <[email protected]>
>
> Add pointer to current and next argument to make parameter processing
> more robust. This can make parameter processing easier and less error
> prone in cases where the parameters need to be enforced/ignored based
> on firmware/system state.
>
> Signed-off-by: Michal Suchanek <[email protected]>
> Signed-off-by: Hari Bathini <[email protected]>
> @@ -179,16 +183,18 @@ char *parse_args(const char *doing,
> if (*args)
> pr_debug("doing %s, parsing ARGS: '%s'\n", doing,
> args);
> - while (*args) {
> + next = next_arg(args, ¶m, &val);
> + while (*next) {
> int ret;
> int irq_was_disabled;
>
> - args = next_arg(args, ¶m, &val);
> + args = next;
> + next = next_arg(args, ¶m, &val);
> /* Stop at -- */
The [PATCH v8 5/6] you refreshed here moves the while(*next) to the end
of the cycle for a reason. Checking *args at the start is mostly
equivalent checking *next at the end. Checking *next at the start on
the other hand skips the last argument.
The "mostly" part is that there is a bug here because *args is not
checked at the start of the cycle making it possible to crash if it is
0. To fix that the if(*args) above should be extended to wrap the cycle.
Thanks
Michal
Actually args are supposed to be renamed to next so both and args hold the
previous argument so both can be passed to the callback. This additionla patch
should fix up the rename.
---
kernel/params.c | 14 ++++++++------
1 file changed, 8 insertions(+), 6 deletions(-)
diff --git a/kernel/params.c b/kernel/params.c
index 69ff58e69887..efb4dfaa6bc5 100644
--- a/kernel/params.c
+++ b/kernel/params.c
@@ -182,17 +182,18 @@ char *parse_args(const char *doing,
if (*args)
pr_debug("doing %s, parsing ARGS: '%s'\n", doing, args);
+ else
+ return err;
- next = next_arg(args, ¶m, &val);
- while (*next) {
+ do {
int ret;
int irq_was_disabled;
- args = next;
next = next_arg(args, ¶m, &val);
+
/* Stop at -- */
if (!val && strcmp(param, "--") == 0)
- return err ?: args;
+ return err ?: next;
irq_was_disabled = irqs_disabled();
ret = parse_one(param, val, args, next, doing, params, num,
min_level, max_level, arg, unknown);
@@ -215,9 +216,10 @@ char *parse_args(const char *doing,
doing, val ?: "", param);
break;
}
-
err = ERR_PTR(ret);
- }
+
+ args = next;
+ } while (*args);
return err;
}
--
2.13.6
On 12/15/2017 01:41 PM, Michal Suchanek wrote:
> Actually args are supposed to be renamed to next so both and args hold the
> previous argument so both can be passed to the callback. This additionla patch
additional
> should fix up the rename.
Would you try rewriting the first sentence, please? I don't get it.
> ---
> kernel/params.c | 14 ++++++++------
> 1 file changed, 8 insertions(+), 6 deletions(-)
--
~Randy
On Fri, 15 Dec 2017 15:49:09 -0800
Randy Dunlap <[email protected]> wrote:
> On 12/15/2017 01:41 PM, Michal Suchanek wrote:
> > Actually args are supposed to be renamed to next so both and args
> > hold the previous argument so both can be passed to the callback.
> > This additionla patch
>
> additional
>
> > should fix up the rename.
>
> Would you try rewriting the first sentence, please? I don't get it.
Ok, I guess this should be clarified. For the original patch and the
fixup squashed together this is what the patch is supposed to do:
This patch adds variable for tracking the parameter which is currently
being processed. There is "args" variable which tracks the parameter
which will be processed next so this patch adds "next" variable to
track that and uses "args" to track the current argument.
Thanks
Michal
On 12/18/2017 09:34 AM, Michal Suchánek wrote:
> On Fri, 15 Dec 2017 15:49:09 -0800
> Randy Dunlap <[email protected]> wrote:
>
>> On 12/15/2017 01:41 PM, Michal Suchanek wrote:
>>> Actually args are supposed to be renamed to next so both and args
>>> hold the previous argument so both can be passed to the callback.
>>> This additionla patch
>>
>> additional
>>
>>> should fix up the rename.
>>
>> Would you try rewriting the first sentence, please? I don't get it.
>
> Ok, I guess this should be clarified. For the original patch and the
> fixup squashed together this is what the patch is supposed to do:
>
> This patch adds variable for tracking the parameter which is currently
> being processed. There is "args" variable which tracks the parameter
> which will be processed next so this patch adds "next" variable to
> track that and uses "args" to track the current argument.
OK, thanks.
--
~Randy