| Message ID | 20180531052915.31171-5-rashmica.g@gmail.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [01/10] libpdbg: Print the name of the instruction when erroring | expand |
On Thursday, 31 May 2018 3:29:10 PM AEST Rashmica Gupta wrote: > The number of arguments required for these functions is wrong - > it requires 2 arguments for putnia and putmsr, and the second one > is written. The code has obviously been copy/pasted from getgpr/getspr > functions where you need to supply which register and then the data. The patch looks ok but I'm having trouble making sense of the commit message. Perhaps something like the below might be clearer? "putnia and putmsr only require 1 argument but the code currently checks for 2 arguments and gets the data from the second argument which is wrong. It has obviously been copy/pasted from putgpr/putspr which do require 2 arguments. Fix this by reducing required arguments to 1 and getting the data from there." - Alistair > Signed-off-by: Rashmica Gupta <rashmica.g@gmail.com> > --- > src/reg.c | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/src/reg.c b/src/reg.c > index 416236b..e252ab8 100644 > --- a/src/reg.c > +++ b/src/reg.c > @@ -152,13 +152,13 @@ int handle_nia(int optind, int argc, char *argv[]) > if (strcmp(argv[optind], "putnia") == 0) { > uint64_t data; > > - if (optind + 2 >= argc) { > + if (optind + 1 >= argc) { > printf("%s: command '%s' requires data\n", argv[0], argv[optind]); > return -1; > } > > errno = 0; > - data = strtoull(argv[optind + 2], &endptr, 0); > + data = strtoull(argv[optind + 1], &endptr, 0); > if (errno || *endptr != '\0') { > printf("%s: command '%s' couldn't parse data '%s'\n", > argv[0], argv[optind], argv[optind + 1]); > @@ -221,13 +221,13 @@ int handle_msr(int optind, int argc, char *argv[]) > if (strcmp(argv[optind], "putmsr") == 0) { > uint64_t data; > > - if (optind + 2 >= argc) { > + if (optind + 1 >= argc) { > printf("%s: command '%s' requires data\n", argv[0], argv[optind]); > return -1; > } > > errno = 0; > - data = strtoull(argv[optind + 2], &endptr, 0); > + data = strtoull(argv[optind + 1], &endptr, 0); > if (errno || *endptr != '\0') { > printf("%s: command '%s' couldn't parse data '%s'\n", > argv[0], argv[optind], argv[optind + 1]); >
On 15/06/18 11:26, Alistair Popple wrote: > On Thursday, 31 May 2018 3:29:10 PM AEST Rashmica Gupta wrote: >> The number of arguments required for these functions is wrong - >> it requires 2 arguments for putnia and putmsr, and the second one >> is written. The code has obviously been copy/pasted from getgpr/getspr >> functions where you need to supply which register and then the data. > The patch looks ok but I'm having trouble making sense of the commit message. > > Perhaps something like the below might be clearer? > > "putnia and putmsr only require 1 argument but the code currently checks for 2 > arguments and gets the data from the second argument which is wrong. It has > obviously been copy/pasted from putgpr/putspr which do require 2 arguments. Fix > this by reducing required arguments to 1 and getting the data from there." That sounds a lot clearer! Do you need me to resend this? > > - Alistair > >> Signed-off-by: Rashmica Gupta <rashmica.g@gmail.com> >> --- >> src/reg.c | 8 ++++---- >> 1 file changed, 4 insertions(+), 4 deletions(-) >> >> diff --git a/src/reg.c b/src/reg.c >> index 416236b..e252ab8 100644 >> --- a/src/reg.c >> +++ b/src/reg.c >> @@ -152,13 +152,13 @@ int handle_nia(int optind, int argc, char *argv[]) >> if (strcmp(argv[optind], "putnia") == 0) { >> uint64_t data; >> >> - if (optind + 2 >= argc) { >> + if (optind + 1 >= argc) { >> printf("%s: command '%s' requires data\n", argv[0], argv[optind]); >> return -1; >> } >> >> errno = 0; >> - data = strtoull(argv[optind + 2], &endptr, 0); >> + data = strtoull(argv[optind + 1], &endptr, 0); >> if (errno || *endptr != '\0') { >> printf("%s: command '%s' couldn't parse data '%s'\n", >> argv[0], argv[optind], argv[optind + 1]); >> @@ -221,13 +221,13 @@ int handle_msr(int optind, int argc, char *argv[]) >> if (strcmp(argv[optind], "putmsr") == 0) { >> uint64_t data; >> >> - if (optind + 2 >= argc) { >> + if (optind + 1 >= argc) { >> printf("%s: command '%s' requires data\n", argv[0], argv[optind]); >> return -1; >> } >> >> errno = 0; >> - data = strtoull(argv[optind + 2], &endptr, 0); >> + data = strtoull(argv[optind + 1], &endptr, 0); >> if (errno || *endptr != '\0') { >> printf("%s: command '%s' couldn't parse data '%s'\n", >> argv[0], argv[optind], argv[optind + 1]); >> >
On Friday, 15 June 2018 1:50:55 PM AEST rashmica wrote: > > On 15/06/18 11:26, Alistair Popple wrote: > > On Thursday, 31 May 2018 3:29:10 PM AEST Rashmica Gupta wrote: > >> The number of arguments required for these functions is wrong - > >> it requires 2 arguments for putnia and putmsr, and the second one > >> is written. The code has obviously been copy/pasted from getgpr/getspr > >> functions where you need to supply which register and then the data. > > The patch looks ok but I'm having trouble making sense of the commit message. > > > > Perhaps something like the below might be clearer? > > > > "putnia and putmsr only require 1 argument but the code currently checks for 2 > > arguments and gets the data from the second argument which is wrong. It has > > obviously been copy/pasted from putgpr/putspr which do require 2 arguments. Fix > > this by reducing required arguments to 1 and getting the data from there." > That sounds a lot clearer! Do you need me to resend this? If you don't mind given there are a few other changes required for this series anyway. - Alistair > > - Alistair > > > >> Signed-off-by: Rashmica Gupta <rashmica.g@gmail.com> > >> --- > >> src/reg.c | 8 ++++---- > >> 1 file changed, 4 insertions(+), 4 deletions(-) > >> > >> diff --git a/src/reg.c b/src/reg.c > >> index 416236b..e252ab8 100644 > >> --- a/src/reg.c > >> +++ b/src/reg.c > >> @@ -152,13 +152,13 @@ int handle_nia(int optind, int argc, char *argv[]) > >> if (strcmp(argv[optind], "putnia") == 0) { > >> uint64_t data; > >> > >> - if (optind + 2 >= argc) { > >> + if (optind + 1 >= argc) { > >> printf("%s: command '%s' requires data\n", argv[0], argv[optind]); > >> return -1; > >> } > >> > >> errno = 0; > >> - data = strtoull(argv[optind + 2], &endptr, 0); > >> + data = strtoull(argv[optind + 1], &endptr, 0); > >> if (errno || *endptr != '\0') { > >> printf("%s: command '%s' couldn't parse data '%s'\n", > >> argv[0], argv[optind], argv[optind + 1]); > >> @@ -221,13 +221,13 @@ int handle_msr(int optind, int argc, char *argv[]) > >> if (strcmp(argv[optind], "putmsr") == 0) { > >> uint64_t data; > >> > >> - if (optind + 2 >= argc) { > >> + if (optind + 1 >= argc) { > >> printf("%s: command '%s' requires data\n", argv[0], argv[optind]); > >> return -1; > >> } > >> > >> errno = 0; > >> - data = strtoull(argv[optind + 2], &endptr, 0); > >> + data = strtoull(argv[optind + 1], &endptr, 0); > >> if (errno || *endptr != '\0') { > >> printf("%s: command '%s' couldn't parse data '%s'\n", > >> argv[0], argv[optind], argv[optind + 1]); > >> > > > >
On 15/06/18 15:12, Alistair Popple wrote: > On Friday, 15 June 2018 1:50:55 PM AEST rashmica wrote: >> On 15/06/18 11:26, Alistair Popple wrote: >>> On Thursday, 31 May 2018 3:29:10 PM AEST Rashmica Gupta wrote: >>>> The number of arguments required for these functions is wrong - >>>> it requires 2 arguments for putnia and putmsr, and the second one >>>> is written. The code has obviously been copy/pasted from getgpr/getspr >>>> functions where you need to supply which register and then the data. >>> The patch looks ok but I'm having trouble making sense of the commit message. >>> >>> Perhaps something like the below might be clearer? >>> >>> "putnia and putmsr only require 1 argument but the code currently checks for 2 >>> arguments and gets the data from the second argument which is wrong. It has >>> obviously been copy/pasted from putgpr/putspr which do require 2 arguments. Fix >>> this by reducing required arguments to 1 and getting the data from there." >> That sounds a lot clearer! Do you need me to resend this? > If you don't mind given there are a few other changes required for this series > anyway. I may have sent this patch separately as well as part of this series... And you may have already merged the one by itself... > > - Alistair > >>> - Alistair >>> >>>> Signed-off-by: Rashmica Gupta <rashmica.g@gmail.com> >>>> --- >>>> src/reg.c | 8 ++++---- >>>> 1 file changed, 4 insertions(+), 4 deletions(-) >>>> >>>> diff --git a/src/reg.c b/src/reg.c >>>> index 416236b..e252ab8 100644 >>>> --- a/src/reg.c >>>> +++ b/src/reg.c >>>> @@ -152,13 +152,13 @@ int handle_nia(int optind, int argc, char *argv[]) >>>> if (strcmp(argv[optind], "putnia") == 0) { >>>> uint64_t data; >>>> >>>> - if (optind + 2 >= argc) { >>>> + if (optind + 1 >= argc) { >>>> printf("%s: command '%s' requires data\n", argv[0], argv[optind]); >>>> return -1; >>>> } >>>> >>>> errno = 0; >>>> - data = strtoull(argv[optind + 2], &endptr, 0); >>>> + data = strtoull(argv[optind + 1], &endptr, 0); >>>> if (errno || *endptr != '\0') { >>>> printf("%s: command '%s' couldn't parse data '%s'\n", >>>> argv[0], argv[optind], argv[optind + 1]); >>>> @@ -221,13 +221,13 @@ int handle_msr(int optind, int argc, char *argv[]) >>>> if (strcmp(argv[optind], "putmsr") == 0) { >>>> uint64_t data; >>>> >>>> - if (optind + 2 >= argc) { >>>> + if (optind + 1 >= argc) { >>>> printf("%s: command '%s' requires data\n", argv[0], argv[optind]); >>>> return -1; >>>> } >>>> >>>> errno = 0; >>>> - data = strtoull(argv[optind + 2], &endptr, 0); >>>> + data = strtoull(argv[optind + 1], &endptr, 0); >>>> if (errno || *endptr != '\0') { >>>> printf("%s: command '%s' couldn't parse data '%s'\n", >>>> argv[0], argv[optind], argv[optind + 1]); >>>> >> >
diff --git a/src/reg.c b/src/reg.c index 416236b..e252ab8 100644 --- a/src/reg.c +++ b/src/reg.c @@ -152,13 +152,13 @@ int handle_nia(int optind, int argc, char *argv[]) if (strcmp(argv[optind], "putnia") == 0) { uint64_t data; - if (optind + 2 >= argc) { + if (optind + 1 >= argc) { printf("%s: command '%s' requires data\n", argv[0], argv[optind]); return -1; } errno = 0; - data = strtoull(argv[optind + 2], &endptr, 0); + data = strtoull(argv[optind + 1], &endptr, 0); if (errno || *endptr != '\0') { printf("%s: command '%s' couldn't parse data '%s'\n", argv[0], argv[optind], argv[optind + 1]); @@ -221,13 +221,13 @@ int handle_msr(int optind, int argc, char *argv[]) if (strcmp(argv[optind], "putmsr") == 0) { uint64_t data; - if (optind + 2 >= argc) { + if (optind + 1 >= argc) { printf("%s: command '%s' requires data\n", argv[0], argv[optind]); return -1; } errno = 0; - data = strtoull(argv[optind + 2], &endptr, 0); + data = strtoull(argv[optind + 1], &endptr, 0); if (errno || *endptr != '\0') { printf("%s: command '%s' couldn't parse data '%s'\n", argv[0], argv[optind], argv[optind + 1]);
The number of arguments required for these functions is wrong - it requires 2 arguments for putnia and putmsr, and the second one is written. The code has obviously been copy/pasted from getgpr/getspr functions where you need to supply which register and then the data. Signed-off-by: Rashmica Gupta <rashmica.g@gmail.com> --- src/reg.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)