diff mbox

sim: Be sure of calling freeargv() after successfully call buildargv().

Message ID 54CC0E99.1070501@sunrus.com.cn
State Committed
Headers

Commit Message

Chen Gang Jan. 30, 2015, 11:07 p.m. UTC 
  Or there will be memory leak.

2014-01-31  Chen Gang <gang.chen.5i5j@gmail.com>

	* microblaze/interp.c (sim_do_command): Call freeargv() before
	return.
---
 sim/ChangeLog           | 5 +++++
 sim/microblaze/interp.c | 3 +++
 2 files changed, 8 insertions(+)

Comments

Michael Eager Jan. 31, 2015, 1:14 a.m. UTC | #1 
On 01/30/15 15:07, Chen Gang S wrote:
> Or there will be memory leak.
>
> 2014-01-31  Chen Gang <gang.chen.5i5j@gmail.com>
>
> 	* microblaze/interp.c (sim_do_command): Call freeargv() before
> 	return.

OK.
Chen Gang Feb. 2, 2015, 7:45 p.m. UTC | #2 
On 1/31/15 09:14, Michael Eager wrote:
> On 01/30/15 15:07, Chen Gang S wrote:
>> Or there will be memory leak.
>>
>> 2014-01-31  Chen Gang <gang.chen.5i5j@gmail.com>
>>

Oh, sorry, the ChangeLog should use 2015-01-31 instead of 2014-01-31 for
the all related 4 patches. I shall send patch v2 for them.


Thanks.
Joel Brobecker Feb. 3, 2015, 2:50 a.m. UTC | #3 
Chen,

> >> 2014-01-31  Chen Gang <gang.chen.5i5j@gmail.com>
> >>
> 
> Oh, sorry, the ChangeLog should use 2015-01-31 instead of 2014-01-31 for
> the all related 4 patches. I shall send patch v2 for them.

One important thing to watch out for: I think you may not have realized
that you pushed all 3 patches, whereas only the last one was approved.
So, the first two ones weren't expected to be pushed yet. If you need
help with git on how to push just the patches you want, let us know.
One way, for instance, is to work in a separate branch, and then
cherry-pick on master the patches only at the time where you want
to push them. That's what I do, and prevents this kind of accident
from happening.

In the meantime, since the patches have been pushed, I reviewed them
and they look good to me. So no need to revert. Normally, those are
reviewed by the sim maintainer, so he may have additional comments.
Chen Gang Feb. 3, 2015, 9:58 a.m. UTC | #4 
On 02/03/2015 10:50 AM, Joel Brobecker wrote:
> Chen,
> 
>>>> 2014-01-31  Chen Gang <gang.chen.5i5j@gmail.com>
>>>>
>>
>> Oh, sorry, the ChangeLog should use 2015-01-31 instead of 2014-01-31 for
>> the all related 4 patches. I shall send patch v2 for them.
> 
> One important thing to watch out for: I think you may not have realized
> that you pushed all 3 patches, whereas only the last one was approved.
> So, the first two ones weren't expected to be pushed yet. If you need
> help with git on how to push just the patches you want, let us know.
> One way, for instance, is to work in a separate branch, and then
> cherry-pick on master the patches only at the time where you want
> to push them. That's what I do, and prevents this kind of accident
> from happening.
> 

OK, thanks. I shall notice about it next time.

> In the meantime, since the patches have been pushed, I reviewed them
> and they look good to me. So no need to revert. Normally, those are
> reviewed by the sim maintainer, so he may have additional comments.
> 

OK, thanks.
Mike Frysinger Feb. 17, 2015, 10:34 a.m. UTC | #5 
On 31 Jan 2015 07:07, Chen Gang S wrote:
> --- a/sim/ChangeLog
> +++ b/sim/ChangeLog
> @@ -1,5 +1,10 @@
>  2014-01-31  Chen Gang <gang.chen.5i5j@gmail.com>
>  
> +	* microblaze/interp.c (sim_do_command): Call freeargv() before
> +	return.

this should be in sim/microblaze/ChangeLog instead.  it looks like your last 4 
entries in sim/ChangeLog need to get relocated.  please do so.

while you're there, you should also fix your gentmap.c entry in 
sim/common/ChangeLog -- only one space after the * is used.

as for the actual code, lgtm.  thanks for fixing up the various error paths.
-mike
Chen Gang Feb. 17, 2015, 11:18 p.m. UTC | #6 
On 2/17/15 18:34, Mike Frysinger wrote:
> On 31 Jan 2015 07:07, Chen Gang S wrote:
>> --- a/sim/ChangeLog
>> +++ b/sim/ChangeLog
>> @@ -1,5 +1,10 @@
>>  2014-01-31  Chen Gang <gang.chen.5i5j@gmail.com>
>>  
>> +	* microblaze/interp.c (sim_do_command): Call freeargv() before
>> +	return.
> 
> this should be in sim/microblaze/ChangeLog instead.  it looks like your last 4 
> entries in sim/ChangeLog need to get relocated.  please do so.
> 

Oh, really, I shall change the related comments.

> while you're there, you should also fix your gentmap.c entry in 
> sim/common/ChangeLog -- only one space after the * is used.
> 

Oh, really, I shall change the related comments.

> as for the actual code, lgtm.  thanks for fixing up the various error paths.
> -mike

That what I should do, since I focus on binutils and gdb. :-)

And excuse me, I am not quite familiar with the related working flow.
Can I send 1 patch to fix the 2 comments, and "git push" it after it is
reviewed?.


Thanks.
Mike Frysinger Feb. 18, 2015, 12:22 a.m. UTC | #7 
On 18 Feb 2015 07:18, Chen Gang S wrote:
> On 2/17/15 18:34, Mike Frysinger wrote:
> > On 31 Jan 2015 07:07, Chen Gang S wrote:
> >> --- a/sim/ChangeLog
> >> +++ b/sim/ChangeLog
> >> @@ -1,5 +1,10 @@
> >>  2014-01-31  Chen Gang <gang.chen.5i5j@gmail.com>
> >>  
> >> +	* microblaze/interp.c (sim_do_command): Call freeargv() before
> >> +	return.
> > 
> > this should be in sim/microblaze/ChangeLog instead.  it looks like your last 4 
> > entries in sim/ChangeLog need to get relocated.  please do so.
> > 
> 
> Oh, really, I shall change the related comments.
> 
> > while you're there, you should also fix your gentmap.c entry in 
> > sim/common/ChangeLog -- only one space after the * is used.
> > 
> 
> Oh, really, I shall change the related comments.
> 
> > as for the actual code, lgtm.  thanks for fixing up the various error paths.
> 
> That what I should do, since I focus on binutils and gdb. :-)
> 
> And excuse me, I am not quite familiar with the related working flow.
> Can I send 1 patch to fix the 2 comments, and "git push" it after it is
> reviewed?.

generally the sim tree follows the gdb/binutils procedure.  i.e. people should 
post patches to the list and wait for approval from a relevant maintainer.  we 
don't generally get strict with the rules as long as you have good intentions 
and aren't breaking things (like compile failures) :).  so don't get too worried 
and feel free to ask as people don't mind helping guide newbies.
-mike
Chen Gang Feb. 19, 2015, 12:41 a.m. UTC | #8 
On 2/18/15 08:22, Mike Frysinger wrote:
> On 18 Feb 2015 07:18, Chen Gang S wrote:
>> On 2/17/15 18:34, Mike Frysinger wrote:
>>> On 31 Jan 2015 07:07, Chen Gang S wrote:
>>>> --- a/sim/ChangeLog
>>>> +++ b/sim/ChangeLog
>>>> @@ -1,5 +1,10 @@
>>>>  2014-01-31  Chen Gang <gang.chen.5i5j@gmail.com>
>>>>  
>>>> +	* microblaze/interp.c (sim_do_command): Call freeargv() before
>>>> +	return.
>>>
>>> this should be in sim/microblaze/ChangeLog instead.  it looks like your last 4 
>>> entries in sim/ChangeLog need to get relocated.  please do so.
>>>
>>
>> Oh, really, I shall change the related comments.
>>
>>> while you're there, you should also fix your gentmap.c entry in 
>>> sim/common/ChangeLog -- only one space after the * is used.
>>>
>>
>> Oh, really, I shall change the related comments.
>>
>>> as for the actual code, lgtm.  thanks for fixing up the various error paths.
>>
>> That what I should do, since I focus on binutils and gdb. :-)
>>
>> And excuse me, I am not quite familiar with the related working flow.
>> Can I send 1 patch to fix the 2 comments, and "git push" it after it is
>> reviewed?.
> 
> generally the sim tree follows the gdb/binutils procedure.  i.e. people should 
> post patches to the list and wait for approval from a relevant maintainer.  we 
> don't generally get strict with the rules as long as you have good intentions 
> and aren't breaking things (like compile failures) :).  so don't get too worried 
> and feel free to ask as people don't mind helping guide newbies.
> -mike
> 

OK, thanks. I shall send one patch for the 2 comments, next.

Thanks.
Michael Eager Feb. 19, 2015, 3:53 p.m. UTC | #9 
On 02/18/15 16:41, Chen Gang S wrote:
> On 2/18/15 08:22, Mike Frysinger wrote:
>> On 18 Feb 2015 07:18, Chen Gang S wrote:
>>> On 2/17/15 18:34, Mike Frysinger wrote:
>>>> On 31 Jan 2015 07:07, Chen Gang S wrote:
>>>>> --- a/sim/ChangeLog
>>>>> +++ b/sim/ChangeLog
>>>>> @@ -1,5 +1,10 @@
>>>>>   2014-01-31  Chen Gang <gang.chen.5i5j@gmail.com>
>>>>>
>>>>> +	* microblaze/interp.c (sim_do_command): Call freeargv() before
>>>>> +	return.
>>>>
>>>> this should be in sim/microblaze/ChangeLog instead.  it looks like your last 4
>>>> entries in sim/ChangeLog need to get relocated.  please do so.
>>>>
>>>
>>> Oh, really, I shall change the related comments.
>>>
>>>> while you're there, you should also fix your gentmap.c entry in
>>>> sim/common/ChangeLog -- only one space after the * is used.
>>>>
>>>
>>> Oh, really, I shall change the related comments.
>>>
>>>> as for the actual code, lgtm.  thanks for fixing up the various error paths.
>>>
>>> That what I should do, since I focus on binutils and gdb. :-)
>>>
>>> And excuse me, I am not quite familiar with the related working flow.
>>> Can I send 1 patch to fix the 2 comments, and "git push" it after it is
>>> reviewed?.
>>
>> generally the sim tree follows the gdb/binutils procedure.  i.e. people should
>> post patches to the list and wait for approval from a relevant maintainer.  we
>> don't generally get strict with the rules as long as you have good intentions
>> and aren't breaking things (like compile failures) :).  so don't get too worried
>> and feel free to ask as people don't mind helping guide newbies.
>> -mike
>>
>
> OK, thanks. I shall send one patch for the 2 comments, next.

If all you are changing is correcting the ChangeLog as indicated,
we can relax the normal procedure.  Please just check in fixes.
diff mbox

Patch

diff --git a/sim/ChangeLog b/sim/ChangeLog
index 1fe93df..582bd4a 100644
--- a/sim/ChangeLog
+++ b/sim/ChangeLog
@@ -1,5 +1,10 @@ 
 2014-01-31  Chen Gang <gang.chen.5i5j@gmail.com>
 
+	* microblaze/interp.c (sim_do_command): Call freeargv() before
+	return.
+
+2014-01-31  Chen Gang <gang.chen.5i5j@gmail.com>
+
 	* mcore/interp.c (sim_do_command): Call freeargv() before return.
 
 2014-01-31  Chen Gang <gang.chen.5i5j@gmail.com>
diff --git a/sim/microblaze/interp.c b/sim/microblaze/interp.c
index 1c8a22d..4fc4595 100644
--- a/sim/microblaze/interp.c
+++ b/sim/microblaze/interp.c
@@ -1019,6 +1019,7 @@  sim_do_command (SIM_DESC sd, const char *cmd)
 	  if ((simargv[1] == NULL) || (simargv[2] == NULL))
 	    {
 	      fprintf (stderr, "Error: missing argument to watch cmd.\n");
+	      freeargv (simargv);
 	      return;
 	    }
 
@@ -1062,6 +1063,8 @@  sim_do_command (SIM_DESC sd, const char *cmd)
 	  fprintf (stderr,"Error: \"%s\" is not a valid M.CORE simulator command.\n",
 		   cmd);
 	}
+
+      freeargv (simargv);
     }
   else
     {