[05/10] pdbg: Tidy up putnia/putmsr

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
Related show

Commit Message

Rashmica Gupta May 31, 2018, 5:29 a.m.
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(-)

Comments

Alistair Popple June 15, 2018, 1:26 a.m. | #1
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]);
>
rashmica June 15, 2018, 3:50 a.m. | #2
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]);
>>
>
Alistair Popple June 15, 2018, 5:12 a.m. | #3
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]);
> >>
> >
> 
>
rashmica June 15, 2018, 7:17 a.m. | #4
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]);
>>>>
>>
>

Patch

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]);