[Openocd-development] [PATCH] Fix for segmentation fault from freed memory access in jtag_unregister_event_callback()

2010-12-07 Thread Paul Richards

Fix for issue described in the following mail

https://lists.berlios.de/pipermail/openocd-development/2010-December/017479.html

From c78f5203622438e73580c327a6e55a0df17b85a8 Mon Sep 17 00:00:00 2001
From: Paul Richards 
Date: Wed, 8 Dec 2010 15:48:55 +0900
Subject: [PATCH] Fix for segmentation fault from freed memory access in 
jtag_unregister_event_callback()

---
 src/jtag/core.c |   22 ++
 1 files changed, 10 insertions(+), 12 deletions(-)

diff --git a/src/jtag/core.c b/src/jtag/core.c
index c1b64bb..4f75e1c 100644
--- a/src/jtag/core.c
+++ b/src/jtag/core.c
@@ -296,27 +296,25 @@ int jtag_register_event_callback(jtag_event_handler_t 
callback, void *priv)
 
 int jtag_unregister_event_callback(jtag_event_handler_t callback, void *priv)
 {
-   struct jtag_event_callback **callbacks_p;
-   struct jtag_event_callback **next;
+   struct jtag_event_callback **callbacks_p = &jtag_event_callbacks;
 
if (callback == NULL)
{
return ERROR_INVALID_ARGUMENTS;
}
 
-   for (callbacks_p = &jtag_event_callbacks;
-   *callbacks_p != NULL;
-   callbacks_p = next)
+   while (*callbacks_p != NULL)
{
-   next = &((*callbacks_p)->next);
-
-   if ((*callbacks_p)->priv != priv)
-   continue;
+   if (((*callbacks_p)->priv != priv) && ((*callbacks_p)->callback 
== callback))
+   {
+   struct jtag_event_callback *free_callback = 
*callbacks_p;
 
-   if ((*callbacks_p)->callback == callback)
+   *callbacks_p = (*callbacks_p)->next;
+   free(free_callback);
+   }
+   else
{
-   free(*callbacks_p);
-   *callbacks_p = *next;
+   callbacks_p = &((*callbacks_p)->next);
}
}
 
-- 
1.7.2.3

___
Openocd-development mailing list
Openocd-development@lists.berlios.de
https://lists.berlios.de/mailman/listinfo/openocd-development


Re: [Openocd-development] [PATCH] Fix for segmentation fault from freed memory access in jtag_unregister_event_callback()

2010-12-07 Thread Peter Stuge
Paul Richards wrote:
> +++ b/src/jtag/core.c
> @@ -296,27 +296,25 @@ int jtag_register_event_callback(jtag_event_handler_t 
> callback, void *priv)
>  
>  int jtag_unregister_event_callback(jtag_event_handler_t callback, void *priv)
>  {
> - struct jtag_event_callback **callbacks_p;
> - struct jtag_event_callback **next;
> + struct jtag_event_callback **callbacks_p = &jtag_event_callbacks;
>  
>   if (callback == NULL)
>   {
>   return ERROR_INVALID_ARGUMENTS;
>   }
>  
> - for (callbacks_p = &jtag_event_callbacks;
> - *callbacks_p != NULL;
> - callbacks_p = next)
> + while (*callbacks_p != NULL)
>   {
> - next = &((*callbacks_p)->next);
> -
> - if ((*callbacks_p)->priv != priv)
> - continue;
> + if (((*callbacks_p)->priv != priv) && ((*callbacks_p)->callback 
> == callback))

The priv comparison logic is reversed.


> + {
> + struct jtag_event_callback *free_callback = 
> *callbacks_p;
>  
> - if ((*callbacks_p)->callback == callback)
> + *callbacks_p = (*callbacks_p)->next;
> + free(free_callback);
> + }
> + else
>   {
> - free(*callbacks_p);
> - *callbacks_p = *next;
> + callbacks_p = &((*callbacks_p)->next);
>   }
>   }

Paul Richards wrote:
> I just saw your mail after submitting a patch.  I kept the double
> pointer usage for consistency but simplified the logic.

The code is horrible. (Certainly not your fault!) Sorry if that's
offensive to the original author. The variable names do not help.
Will this work:

int jtag_unregister_event_callback(callback,priv) {
  struct jtag_event_callback *p, *prev = NULL, *next;

  if (!callback)
return ERROR_INVALID_ARGUMENTS;

  for (p = jtag_event_callbacks; p; p = next) {
next = p->next;

if (p->priv != priv || p->callback != callback) {
  prev = p;
  continue;
}

if (prev)
  prev->next = next;
else
  jtag_events_callbacks = next;

free(p);
  }
}

?


//Peter
___
Openocd-development mailing list
Openocd-development@lists.berlios.de
https://lists.berlios.de/mailman/listinfo/openocd-development


Re: [Openocd-development] [PATCH] Fix for segmentation fault from freed memory access in jtag_unregister_event_callback()

2010-12-08 Thread Paul Richards

On 2010/12/08 16:27, Peter Stuge wrote:


The priv comparison logic is reversed.


Sorry, can't believe that snuck in, thanks.


The code is horrible. (Certainly not your fault!) Sorry if that's
offensive to the original author. The variable names do not help.
Will this work:

int jtag_unregister_event_callback(callback,priv) {
   struct jtag_event_callback *p, *prev = NULL, *next;

   if (!callback)
 return ERROR_INVALID_ARGUMENTS;

   for (p = jtag_event_callbacks; p; p = next) {
 next = p->next;

 if (p->priv != priv || p->callback != callback) {
   prev = p;
   continue;
 }

 if (prev)
   prev->next = next;
 else
   jtag_events_callbacks = next;

 free(p);
   }
}



How about the following, it cleans up the naming, but does keep the 
double pointer usage which I think leaves it more compact, but debatably 
harder to read?


int jtag_unregister_event_callback(jtag_event_handler_t callback, void 
*priv)

{
struct jtag_event_callback **p = &jtag_event_callbacks, *temp;

if (callback == NULL)
{
return ERROR_INVALID_ARGUMENTS;
}

while (*p)
{
if (((*p)->priv != priv) || ((*p)->callback != callback))
{
p = &(*p)->next;
continue;
}

temp = *p;
*p = (*p)->next;
free(temp);
}

return ERROR_OK;
}

I'm happy to remove the the double pointer if you prefer, I personally 
prefer the elegance in the compactness, but I'm happy to go along with 
what the pereferences are here.


Also I note that you use the K&R bracing, though most of the other code 
in this file doesn't.  Is there a preference?


Thanks,

Paul

___
Openocd-development mailing list
Openocd-development@lists.berlios.de
https://lists.berlios.de/mailman/listinfo/openocd-development


Re: [Openocd-development] [PATCH] Fix for segmentation fault from freed memory access in jtag_unregister_event_callback()

2010-12-08 Thread Paul Richards
I'm attaching the patch proposed in the last mail as I'll be offline 
shortly.


Regards,

Paul

On 2010/12/08 18:02, Paul Richards wrote:

On 2010/12/08 16:27, Peter Stuge wrote:


The priv comparison logic is reversed.


Sorry, can't believe that snuck in, thanks.


The code is horrible. (Certainly not your fault!) Sorry if that's
offensive to the original author. The variable names do not help.
Will this work:

int jtag_unregister_event_callback(callback,priv) {
   struct jtag_event_callback *p, *prev = NULL, *next;

   if (!callback)
 return ERROR_INVALID_ARGUMENTS;

   for (p = jtag_event_callbacks; p; p = next) {
 next = p->next;

 if (p->priv != priv || p->callback != callback) {
   prev = p;
   continue;
 }

 if (prev)
   prev->next = next;
 else
   jtag_events_callbacks = next;

 free(p);
   }
}



How about the following, it cleans up the naming, but does keep the 
double pointer usage which I think leaves it more compact, but 
debatably harder to read?


int jtag_unregister_event_callback(jtag_event_handler_t callback, void 
*priv)

{
struct jtag_event_callback **p = &jtag_event_callbacks, *temp;

if (callback == NULL)
{
return ERROR_INVALID_ARGUMENTS;
}

while (*p)
{
if (((*p)->priv != priv) || ((*p)->callback != callback))
{
p = &(*p)->next;
continue;
}

temp = *p;
*p = (*p)->next;
free(temp);
}

return ERROR_OK;
}

I'm happy to remove the the double pointer if you prefer, I personally 
prefer the elegance in the compactness, but I'm happy to go along with 
what the pereferences are here.


Also I note that you use the K&R bracing, though most of the other 
code in this file doesn't.  Is there a preference?


Thanks,

Paul



From 987ecc00efaf0ee4dd2fae7b68bf4cf5a4272902 Mon Sep 17 00:00:00 2001
From: Paul Richards 
Date: Wed, 8 Dec 2010 15:48:55 +0900
Subject: [PATCH] Fix for segmentation fault from freed memory access in 
jtag_unregister_event_callback()

---
 src/jtag/core.c |   22 +-
 1 files changed, 9 insertions(+), 13 deletions(-)

diff --git a/src/jtag/core.c b/src/jtag/core.c
index c1b64bb..f3408e1 100644
--- a/src/jtag/core.c
+++ b/src/jtag/core.c
@@ -296,28 +296,24 @@ int jtag_register_event_callback(jtag_event_handler_t 
callback, void *priv)
 
 int jtag_unregister_event_callback(jtag_event_handler_t callback, void *priv)
 {
-   struct jtag_event_callback **callbacks_p;
-   struct jtag_event_callback **next;
+   struct jtag_event_callback **p = &jtag_event_callbacks, *temp;
 
if (callback == NULL)
{
return ERROR_INVALID_ARGUMENTS;
}
 
-   for (callbacks_p = &jtag_event_callbacks;
-   *callbacks_p != NULL;
-   callbacks_p = next)
+   while (*p)
{
-   next = &((*callbacks_p)->next);
-
-   if ((*callbacks_p)->priv != priv)
-   continue;
-
-   if ((*callbacks_p)->callback == callback)
+   if (((*p)->priv != priv) || ((*p)->callback != callback))
{
-   free(*callbacks_p);
-   *callbacks_p = *next;
+   p = &(*p)->next;
+   continue;
}
+
+   temp = *p;
+   *p = (*p)->next;
+   free(temp);
}
 
return ERROR_OK;
-- 
1.7.2.3

___
Openocd-development mailing list
Openocd-development@lists.berlios.de
https://lists.berlios.de/mailman/listinfo/openocd-development


Re: [Openocd-development] [PATCH] Fix for segmentation fault from freed memory access in jtag_unregister_event_callback()

2010-12-09 Thread Øyvind Harboe
Merged.

Thanks!



-- 
Øyvind Harboe

Can Zylin Consulting help on your project?

US toll free 1-866-980-3434 / International +47 51 63 25 00

http://www.zylin.com/zy1000.html
ARM7 ARM9 ARM11 XScale Cortex
JTAG debugger and flash programmer
___
Openocd-development mailing list
Openocd-development@lists.berlios.de
https://lists.berlios.de/mailman/listinfo/openocd-development


Re: [Openocd-development] [PATCH] Fix for segmentation fault from freed memory access in jtag_unregister_event_callback()

2010-12-09 Thread Peter Stuge
Øyvind Harboe wrote:
> Merged.

So I should have complained more loudly. The double pointers are
horrible.


//Peter
___
Openocd-development mailing list
Openocd-development@lists.berlios.de
https://lists.berlios.de/mailman/listinfo/openocd-development


Re: [Openocd-development] [PATCH] Fix for segmentation fault from freed memory access in jtag_unregister_event_callback()

2010-12-09 Thread Øyvind Harboe
On Fri, Dec 10, 2010 at 8:05 AM, Peter Stuge  wrote:
> Øyvind Harboe wrote:
>> Merged.
>
> So I should have complained more loudly. The double pointers are
> horrible.

Ah, missed that. Well another patch is gladly accepted, meanwhile
it no longer crashes. Hopefully the fact that I accepted a patch, won't
completely kill any motivation to do some more cleanup.

I must admit I had to read over it a couple of times, but I didn't
want to touch the code as I didn't have a way to readily
reproduce/test it.

-- 
Øyvind Harboe

Can Zylin Consulting help on your project?

US toll free 1-866-980-3434 / International +47 51 63 25 00

http://www.zylin.com/zy1000.html
ARM7 ARM9 ARM11 XScale Cortex
JTAG debugger and flash programmer
___
Openocd-development mailing list
Openocd-development@lists.berlios.de
https://lists.berlios.de/mailman/listinfo/openocd-development


Re: [Openocd-development] [PATCH] Fix for segmentation fault from freed memory access in jtag_unregister_event_callback()

2010-12-09 Thread Peter Stuge
Øyvind Harboe wrote:
> > double pointers are horrible.
> 
> Ah, missed that. Well another patch is gladly accepted, meanwhile
> it no longer crashes.

Fair. Two versions attached. One which makes the code two lines
longer and avoids double pointers, and a remix which uses only a
single double pointer and is net one line shorter.


//Peter
>From 2794909b0654630d71fe0fd618d7188a450234c4 Mon Sep 17 00:00:00 2001
From: Peter Stuge 
Date: Fri, 10 Dec 2010 08:19:23 +0100
Subject: [PATCH] jtag: Avoid double pointers when removing event callbacks

---
 src/jtag/core.c |   22 --
 1 files changed, 12 insertions(+), 10 deletions(-)

diff --git a/src/jtag/core.c b/src/jtag/core.c
index dfedc17..dd6c8d0 100644
--- a/src/jtag/core.c
+++ b/src/jtag/core.c
@@ -296,24 +296,26 @@ int jtag_register_event_callback(jtag_event_handler_t 
callback, void *priv)
 
 int jtag_unregister_event_callback(jtag_event_handler_t callback, void *priv)
 {
-   struct jtag_event_callback **p = &jtag_event_callbacks, *temp;
+   struct jtag_event_callback *p, *prev, *next;
 
-   if (callback == NULL)
-   {
+   if (!callback)
return ERROR_INVALID_ARGUMENTS;
-   }
 
-   while (*p)
+   for (p = jtag_event_callbacks, prev = NULL, p, p = next)
{
-   if (((*p)->priv != priv) || ((*p)->callback != callback))
+   next = p->next;
+
+   if (p->callback != callback || p->priv != priv)
{
-   p = &(*p)->next;
+   prev = p;
continue;
}
 
-   temp = *p;
-   *p = (*p)->next;
-   free(temp);
+   if (prev)
+   prev->next = next;
+   else
+   jtag_event_callbacks = next;
+   free(p);
}
 
return ERROR_OK;
-- 
1.7.2

>From 1063bf4e9dcb722f39f015af47864b3583e6bdb1 Mon Sep 17 00:00:00 2001
From: Peter Stuge 
Date: Fri, 10 Dec 2010 08:19:23 +0100
Subject: [PATCH] jtag: Use only one double pointer when removing event callbacks

---
 src/jtag/core.c |   19 +--
 1 files changed, 9 insertions(+), 10 deletions(-)

diff --git a/src/jtag/core.c b/src/jtag/core.c
index dfedc17..6a97079 100644
--- a/src/jtag/core.c
+++ b/src/jtag/core.c
@@ -296,24 +296,23 @@ int jtag_register_event_callback(jtag_event_handler_t 
callback, void *priv)
 
 int jtag_unregister_event_callback(jtag_event_handler_t callback, void *priv)
 {
-   struct jtag_event_callback **p = &jtag_event_callbacks, *temp;
+   struct jtag_event_callback *p, *prev, *next;
 
-   if (callback == NULL)
-   {
+   if (!callback)
return ERROR_INVALID_ARGUMENTS;
-   }
 
-   while (*p)
+   for (p = jtag_event_callbacks, prev = NULL, p, p = next)
{
-   if (((*p)->priv != priv) || ((*p)->callback != callback))
+   next = p->next;
+
+   if (p->callback != callback || p->priv != priv)
{
-   p = &(*p)->next;
+   prev = p;
continue;
}
 
-   temp = *p;
-   *p = (*p)->next;
-   free(temp);
+   *(prev ? &prev->next : &jtag_event_callbacks) = next;
+   free(p);
}
 
return ERROR_OK;
-- 
1.7.2

___
Openocd-development mailing list
Openocd-development@lists.berlios.de
https://lists.berlios.de/mailman/listinfo/openocd-development


Re: [Openocd-development] [PATCH] Fix for segmentation fault from freed memory access in jtag_unregister_event_callback()

2010-12-09 Thread Øyvind Harboe
On Fri, Dec 10, 2010 at 8:27 AM, Peter Stuge  wrote:
> Øyvind Harboe wrote:
>> > double pointers are horrible.
>>
>> Ah, missed that. Well another patch is gladly accepted, meanwhile
>> it no longer crashes.
>
> Fair. Two versions attached. One which makes the code two lines
> longer and avoids double pointers, and a remix which uses only a
> single double pointer and is net one line shorter.

I'll leave it to you to decide which is the best way to
implement this and to incorporate any feedback. Give the word
when it is ready to be committed and which patch is
the winner.


-- 
Øyvind Harboe

Can Zylin Consulting help on your project?

US toll free 1-866-980-3434 / International +47 51 63 25 00

http://www.zylin.com/zy1000.html
ARM7 ARM9 ARM11 XScale Cortex
JTAG debugger and flash programmer
___
Openocd-development mailing list
Openocd-development@lists.berlios.de
https://lists.berlios.de/mailman/listinfo/openocd-development


Re: [Openocd-development] [PATCH] Fix for segmentation fault from freed memory access in jtag_unregister_event_callback()

2010-12-10 Thread Peter Stuge
Øyvind Harboe wrote:
> > Two versions attached.
> 
> I'll leave it to you to decide which is the best way to
> implement this

I prefer the longer but simpler to read one. Were it mostly a smaller
my own project then I'd gone with the shorter one.


> and to incorporate any feedback.

I doubt anyone else will care about it. :)


> Give the word when it is ready to be committed and which patch is
> the winner.

Let's see if there's any comment.


//Peter
___
Openocd-development mailing list
Openocd-development@lists.berlios.de
https://lists.berlios.de/mailman/listinfo/openocd-development


Re: [Openocd-development] [PATCH] Fix for segmentation fault from freed memory access in jtag_unregister_event_callback()

2010-12-10 Thread Øyvind Harboe
On Fri, Dec 10, 2010 at 9:28 AM, Peter Stuge  wrote:
> Øyvind Harboe wrote:
>> > Two versions attached.
>>
>> I'll leave it to you to decide which is the best way to
>> implement this
>
> I prefer the longer but simpler to read one. Were it mostly a smaller
> my own project then I'd gone with the shorter one.

I think optimizing for testability & readability is the way to go here.

Especially considering the evidence that this code doesn't get tested
a whole lot


-- 
Øyvind Harboe

Can Zylin Consulting help on your project?

US toll free 1-866-980-3434 / International +47 51 63 25 00

http://www.zylin.com/zy1000.html
ARM7 ARM9 ARM11 XScale Cortex
JTAG debugger and flash programmer
___
Openocd-development mailing list
Openocd-development@lists.berlios.de
https://lists.berlios.de/mailman/listinfo/openocd-development


Re: [Openocd-development] [PATCH] Fix for segmentation fault from freed memory access in jtag_unregister_event_callback()

2010-12-10 Thread Spencer Oliver

On 10/12/2010 08:34, Øyvind Harboe wrote:

On Fri, Dec 10, 2010 at 9:28 AM, Peter Stuge  wrote:

Øyvind Harboe wrote:

Two versions attached.


I'll leave it to you to decide which is the best way to
implement this


I prefer the longer but simpler to read one. Were it mostly a smaller
my own project then I'd gone with the shorter one.


I think optimizing for testability&  readability is the way to go here.

Especially considering the evidence that this code doesn't get tested
a whole lot




Not looked into it but why do we not just duplicate the existing 
unregister event/timer functions - or are they broken aswell?


Cheers
Spen
___
Openocd-development mailing list
Openocd-development@lists.berlios.de
https://lists.berlios.de/mailman/listinfo/openocd-development


Re: [Openocd-development] [PATCH] Fix for segmentation fault from freed memory access in jtag_unregister_event_callback()

2010-12-10 Thread Paul Richards

On 2010/12/10 17:34, Øyvind Harboe wrote:

I think optimizing for testability & readability is the way to go here.
Especially considering the evidence that this code doesn't get tested
a whole lot



I can verify that both solutions work.

I'm OK with whichever solution you guys choose since you've been around 
here a lot longer than I :-)


Cheers,

Paul

___
Openocd-development mailing list
Openocd-development@lists.berlios.de
https://lists.berlios.de/mailman/listinfo/openocd-development


Re: [Openocd-development] [PATCH] Fix for segmentation fault from freed memory access in jtag_unregister_event_callback()

2010-12-10 Thread Paul Richards

On 2010/12/10 18:46, Spencer Oliver wrote:
Not looked into it but why do we not just duplicate the existing 
unregister event/timer functions - or are they broken aswell?




They (the target versions) don't appear to have the same problem.  The 
only difference I can see is that the jtag_unregister_event_callback() 
continues to look for further events to unregister.  I'm not familiar 
enough with the code to know why that might be possible.  Easy enough to 
continue iterating if it is required.


Now there's 3 versions to choose from :-)  I would have copied these had 
I known they were there.


Cheers,

Paul

___
Openocd-development mailing list
Openocd-development@lists.berlios.de
https://lists.berlios.de/mailman/listinfo/openocd-development


Re: [Openocd-development] [PATCH] Fix for segmentation fault from freed memory access in jtag_unregister_event_callback()

2010-12-10 Thread Andreas Fritiofson
On Fri, Dec 10, 2010 at 4:16 PM, Paul Richards  wrote:
> On 2010/12/10 18:46, Spencer Oliver wrote:
>>
>> Not looked into it but why do we not just duplicate the existing
>> unregister event/timer functions - or are they broken aswell?
>>
>
> They (the target versions) don't appear to have the same problem.  The only
> difference I can see is that the jtag_unregister_event_callback() continues
> to look for further events to unregister.  I'm not familiar enough with the
> code to know why that might be possible.  Easy enough to continue iterating
> if it is required.
>
> Now there's 3 versions to choose from :-)  I would have copied these had I
> known they were there.
>

It's not too late. The target versions seems nice and readable. And
correct, as far as I can see this late hour. I think the jtag version
should follow the behavior of these and only remove the first matching
handler. That would be the correct thing to do, if there was a point
in having the same handler registered more than once.

The only issue I have with copying the target version is that having
the same code duplicated in three places probably warrants refactoring
it into a helper function instead.

Regards,
Andreas
___
Openocd-development mailing list
Openocd-development@lists.berlios.de
https://lists.berlios.de/mailman/listinfo/openocd-development


Re: [Openocd-development] [PATCH] Fix for segmentation fault from freed memory access in jtag_unregister_event_callback()

2010-12-10 Thread Peter Stuge
Andreas Fritiofson wrote:
> > Now there's 3 versions to choose from :-)
> 
> It's not too late. The target versions seems nice and readable. And
> correct, as far as I can see this late hour. I think the jtag version
> should follow the behavior of these and only remove the first matching
> handler. That would be the correct thing to do, if there was a point
> in having the same handler registered more than once.

Really? How are the callbacks being used?


> The only issue I have with copying the target version is that having
> the same code duplicated in three places probably warrants refactoring
> it into a helper function instead.

Which is difficult for something simple as a linked list, unless the
list entries are identical, and of the same type. Are they?


//Peter
___
Openocd-development mailing list
Openocd-development@lists.berlios.de
https://lists.berlios.de/mailman/listinfo/openocd-development


Re: [Openocd-development] [PATCH] Fix for segmentation fault from freed memory access in jtag_unregister_event_callback()

2010-12-10 Thread Andreas Fritiofson
On Fri, Dec 10, 2010 at 11:39 PM, Peter Stuge  wrote:
> Andreas Fritiofson wrote:
>> > Now there's 3 versions to choose from :-)
>>
>> It's not too late. The target versions seems nice and readable. And
>> correct, as far as I can see this late hour. I think the jtag version
>> should follow the behavior of these and only remove the first matching
>> handler. That would be the correct thing to do, if there was a point
>> in having the same handler registered more than once.
>
> Really? How are the callbacks being used?

I don't know. But, generally, if I _can_ register the same handler
twice, I'd expect each registration to require a separate unregister
call. The actual call sites would have to be inspected before changing
this behavior, of course. There shouldn't be many of them, since the
bug has gone unnoticed for so long.

>> The only issue I have with copying the target version is that having
>> the same code duplicated in three places probably warrants refactoring
>> it into a helper function instead.
>
> Which is difficult for something simple as a linked list, unless the
> list entries are identical, and of the same type. Are they?

Two of them differ only in the callback signature, the third is very different.

There are several imaginable solutions. You could have the list
functions operate on a generic next pointer, included as the first
element of all specialized list types. Or the actual data could be
stored outside the list structure, with just a void pointer in it.
Both require some nasty casts. Neither is likely to be more clear and
concise than the duplicated code. Maybe if it was used in _ten_
places...

/Andreas
___
Openocd-development mailing list
Openocd-development@lists.berlios.de
https://lists.berlios.de/mailman/listinfo/openocd-development


Re: [Openocd-development] [PATCH] Fix for segmentation fault from freed memory access in jtag_unregister_event_callback()

2010-12-10 Thread Paul Richards

On 2010/12/11 8:20, Andreas Fritiofson wrote:


I don't know. But, generally, if I _can_ register the same handler
twice, I'd expect each registration to require a separate unregister
call. The actual call sites would have to be inspected before changing
this behavior, of course. There shouldn't be many of them, since the
bug has gone unnoticed for so long.



Looking at this further, there are only two places that 
jtag_register_event_callback() is called from, the first from 
jtag_tap_init() with the a new TAP instance as the priv parameter, and 
the second from target_examine() with a target as the priv parameter.


The first case I believe can only be called once per unique priv 
instance by virtue of the fact that it's part of the TAP initialisation 
and the TAP instance is the priv parameter.  The second case only 
appears to be called from the init handler which is guarded by a static 
initialized flag.


However, there is nothing to help prevent issues being introduced with 
their future usage.  At the very least the functions should have banner 
describing the interface contract, specifically whether multiple 
registration are disallowed, allowed with balanced de-registration, or 
allowed with only a single de-registration necessary.  I think the 
former with safety checks would be the safest, however this would 
increase the footprint of change if we were to require consistency.


For now I suggest we can stay consistent with the target based list 
function implementations?  This would appear to imply that multiple 
registrations are possible but registration / de-registration balance is 
the responsibility of the client.


I also think the benefits of a common list function here are outweighed 
by the simplicity of the function (the bug and the generated discussion 
excepted!) and lost readability in the specialisation casts.


I'd be happy to test and submit a new patch candidate if and when we're 
all in agreement.


Peter, note that the double pointers are in the target list function 
implementations (that would be copied), are you ok with this?  I think 
the implementations are concise and readable, though maybe my 
sensitivity to double pointers has dulled...


Cheers,

Paul

___
Openocd-development mailing list
Openocd-development@lists.berlios.de
https://lists.berlios.de/mailman/listinfo/openocd-development


Re: [Openocd-development] [PATCH] Fix for segmentation fault from freed memory access in jtag_unregister_event_callback()

2010-12-10 Thread Peter Stuge
Paul Richards wrote:
> Peter, note that the double pointers are in the target list function 
> implementations (that would be copied), are you ok with this?

Afraid not. The alternative is quite straightforward, so I would
strongly prefer not going there.

Thanks!


//Peter
___
Openocd-development mailing list
Openocd-development@lists.berlios.de
https://lists.berlios.de/mailman/listinfo/openocd-development


Re: [Openocd-development] [PATCH] Fix for segmentation fault from freed memory access in jtag_unregister_event_callback()

2010-12-10 Thread Paul Richards

On 2010/12/11 13:00, Peter Stuge wrote:

Afraid not. The alternative is quite straightforward, so I would
strongly prefer not going there.


Thought as much :-)

Well, will leave it up to you guys to decide.  I can test any new 
patches required.


Cheers,

Paul

___
Openocd-development mailing list
Openocd-development@lists.berlios.de
https://lists.berlios.de/mailman/listinfo/openocd-development


Re: [Openocd-development] [PATCH] Fix for segmentation fault from freed memory access in jtag_unregister_event_callback()

2010-12-10 Thread Peter Stuge
Peter Stuge wrote:
> The alternative is quite straightforward, so I would
> strongly prefer not going there.

Allow me to clarify that;

The alternative of rewriting to use no double pointers makes the code
much more straightforward, so I would strongly prefer not using any
double pointers. :)


//Peter
___
Openocd-development mailing list
Openocd-development@lists.berlios.de
https://lists.berlios.de/mailman/listinfo/openocd-development