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.
> 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]>
> 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();
> + 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'.
> - }
>
> *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.
> + 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");
>
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");
>>
>