On 06/01/2015 11:28 AM, Daniel Thompson wrote:
> On 31/05/15 18:59, Filip Ayazi wrote:
>> Replace simple_strto with appropriate kstrto functions as recommended
>> by checkpatch, change pid, sig types to unsigned int, int respectively.
>
> A changelog describing the changes are in v2 would be nice here.
My apologies, I will certainly include a changelog in the next version.
>
>> Signed-off-by: Filip Ayazi <[email protected]>
>> ---
>> kernel/debug/kdb/kdb_main.c | 56
>> +++++++++++++++------------------------------
>> 1 file changed, 19 insertions(+), 37 deletions(-)
>
> I can't find any bugs introduced by this patch although I do have some
> cosmetic nitpicks below. You can add my reviewed-by to the next release:
>
> Reviewed-by: Daniel Thompson <[email protected]>
Thanks.
>
>> diff --git a/kernel/debug/kdb/kdb_main.c b/kernel/debug/kdb/kdb_main.c
>> index 4121345..6458330 100644
>> --- a/kernel/debug/kdb/kdb_main.c
>> +++ b/kernel/debug/kdb/kdb_main.c
>> @@ -296,7 +296,8 @@ static int kdbgetulenv(const char *match, unsigned long
>> *value)
>> if (strlen(ep) == 0)
>> return KDB_NOENVVALUE;
>>
>> - *value = simple_strtoul(ep, NULL, 0);
>> + if (kstrtoul(ep, 0, value) != 0)
> ^^^^^
> Checking for != 0 is what if does anyway. A generally clearer code pattern
> for kernel code that returns 0 on success is:
>
> err = kstrtoul(...);
> if (err)
> handle_error();
>
I wasn't sure whether to write the != 0 as it has no function there,
but it seemed more clear with it (as I am used to returning true on success).
It will be fixed in the next version.
>
>> + return KDB_BADINT;
>>
>> return 0;
>> }
>> @@ -334,20 +335,15 @@ int kdbgetintenv(const char *match, int *value)
>> */
>> int kdbgetularg(const char *arg, unsigned long *value)
>> {
>> - char *endp;
>> unsigned long val;
>>
>> - val = simple_strtoul(arg, &endp, 0);
>> -
>> - if (endp == arg) {
>> - /*
>> - * Also try base 16, for us folks too lazy to type the
>> - * leading 0x...
>> - */
>> - val = simple_strtoul(arg, &endp, 16);
>> - if (endp == arg)
>> + /*
>> + * Also try base 16, for us folks too lazy to type the
>> + * leading 0x...
>> + */
>> + if (kstrtoul(arg, 0, &val) != 0)
>> + if (kstrtoul(arg, 16, &val) != 0)
>> return KDB_BADINT;
>
> Whether or not it is good taste to introduce 'err' here is debatable since it
> makes the code flow pretty verbose. However it would still be good to get rid
> of the '!= 0'.
I'd prefer not to introduce err, as I believe its quite clear what the code
does.
>
>> - }
>>
>> *value = val;
>
> kstrtoul() does not write to its argument unless it is successful. That means
> we no longer need a temporary variable here; just pass value into kstrtoul().
>
>
>> @@ -356,17 +352,11 @@ int kdbgetularg(const char *arg, unsigned long *value)
>>
>> int kdbgetu64arg(const char *arg, u64 *value)
>> {
>> - char *endp;
>> u64 val;
>>
>> - val = simple_strtoull(arg, &endp, 0);
>> -
>> - if (endp == arg) {
>> -
>> - val = simple_strtoull(arg, &endp, 16);
>> - if (endp == arg)
>> + if (kstrtoull(arg, 0, &val) != 0)
>> + if (kstrtoull(arg, 16, &val) != 0)
>> return KDB_BADINT;
>
> +1
>
>
>> - }
>>
>> *value = val;
>>
>> @@ -402,10 +392,9 @@ int kdb_set(int argc, const char **argv)
>> */
>> if (strcmp(argv[1], "KDBDEBUG") == 0) {
>> unsigned int debugflags;
>> - char *cp;
>>
>> - debugflags = simple_strtoul(argv[2], &cp, 0);
>> - if (cp == argv[2] || debugflags & ~KDB_DEBUG_FLAG_MASK) {
>> + if (kstrtouint(argv[2], 0, &debugflags) != 0
>> + || debugflags & ~KDB_DEBUG_FLAG_MASK) {
>
> +1
>
>
>> kdb_printf("kdb: illegal debug flags '%s'\n",
>> argv[2]);
>> return 0;
>> @@ -1588,10 +1577,8 @@ static int kdb_md(int argc, const char **argv)
>> if (!argv[0][3])
>> valid = 1;
>> else if (argv[0][3] == 'c' && argv[0][4]) {
>> - char *p;
>> - repeat = simple_strtoul(argv[0] + 4, &p, 10);
>> + valid = !kstrtoint(argv[0]+4, 10, &repeat);
>> mdcount = ((repeat * bytesperword) + 15) / 16;
>> - valid = !*p;
>> }
>> last_repeat = repeat;
>
> Interesting! Your changes mean we no longer store the, known invalid, return
> value from simple_strtoul into last_repeat. Since last_repeat is a static
> variable and can influence future commands that's a *good* change.
>
>
>> } else if (strcmp(argv[0], "md") == 0)
>> @@ -2091,13 +2078,10 @@ static int kdb_dmesg(int argc, const char **argv)
>> if (argc > 2)
>> return KDB_ARGCOUNT;
>> if (argc) {
>> - char *cp;
>> - lines = simple_strtol(argv[1], &cp, 0);
>> - if (*cp)
>> + if (kstrtoint(argv[1], 0, &lines) != 0)
>> lines = 0;
>> if (argc > 1) {
>> - adjust = simple_strtoul(argv[2], &cp, 0);
>> - if (*cp || adjust < 0)
>> + if (kstrtoint(argv[2], 0, &adjust) != 0 || adjust < 0)
>> adjust = 0;
>
> +2
>
>
>> }
>> }
>> @@ -2436,16 +2420,15 @@ static int kdb_help(int argc, const char **argv)
>> */
>> static int kdb_kill(int argc, const char **argv)
>> {
>> - long sig, pid;
>> - char *endp;
>> + unsigned int pid;
>
> Why not use pid_t/kstrtoint() here? The code already has a check to cope with
> the user passing a negative value.
>
Good point, I will change it to pid_t in the next version.
Thanks for reviewing the patch, I will incorporate all these changes and
send a new version soon.
>
>> + int sig;
>> struct task_struct *p;
>> struct siginfo info;
>>
>> if (argc != 2)
>> return KDB_ARGCOUNT;
>>
>> - sig = simple_strtol(argv[1], &endp, 0);
>> - if (*endp)
>> + if (kstrtoint(argv[1], 0, &sig) != 0)
>> return KDB_BADINT;
>> if (sig >= 0) {
>> kdb_printf("Invalid signal parameter.<-signal>\n");
>> @@ -2453,8 +2436,7 @@ static int kdb_kill(int argc, const char **argv)
>> }
>> sig = -sig;
>>
>> - pid = simple_strtol(argv[2], &endp, 0);
>> - if (*endp)
>> + if (kstrtouint(argv[2], 0, &pid) != 0)
>> return KDB_BADINT;
>> if (pid <= 0) {
>> kdb_printf("Process ID must be large than 0.\n");
>>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
