Re: [Openipmi-developer] [PATCH] Question regarding a change between 1.4.25 and 1.4.26 (still present in 2.0.13)

[Corey M. <mi...@ac...>]

Comments on the patch inline.  I have no idea why those changes would 
make anything work.  The first change might result in a memory leak, but 
that's it.  The second change is clearly wrong and will almost always 
result in a double unlock.

You are sure that you apply this patch and it starts working correctly, 
then remove it and it stops?  I can't understand why those changes would 
make a difference.

-corey

Jonathan Fournier wrote:
> Hi,
>
> I have an "issue" I'm able to fix by backtracking a change in OpenIPMI
> but I don't understand why it fixes the thing, if possible I would
> like to know about that change.
>
> I'm using an older revision of OpenIPMI (1.4.26) and openhpi/libipmi
> plugin (2.2.1).
>
> When I generate sensor threshold events I don't see the events in
> hpigetevents or hpi_cmd/event enable. But the events are actually
> displayed by runing hpiel. I spent some time trying to figure out
> where the issue was (openhpi, OpenIPMI, the IPMI driver etc) and
> managed to find a change that introduced that behavior (I tried a lot
> of hpi/ipmi packages combos from older to latest ones). The change
> that introduced that (see the patch to backtrack it) was the commit on
> 2006-02-28 09:50
>
> 2006-02-28 09:50  cminyard
>
>    * ChangeLog (1.527.2.134), lib/chassis.c (1.20.2.2), lib/mc.c
>      (1.88.2.22), ChangeLog (1.756), lib/chassis.c (1.22), lib/mc.c
>      (1.115): More minor fixes.
>
> 2006-02-28 03:54  cminyard
>
>    * lib/: mc.c (1.114), mc.c (1.88.2.21): Missed something in the
>      last change.
>
> Any comments on that change? It's still present in 2.0.13. I would
> like to know if my IPMC could be doing something wrong? The
> combination of the spinlock move and the call to
> ipmi_mc_remove_active_handler() introduced the error in my case. By
> backtracking a subset of the changes as it was on 2006-02-28 03:54
> with that patch the problem goes away.
>
> Regards,
>
> /jonathan
>
> --- CUT HERE ---
>
> Signed-off-by: Jonathan Fournier <jon...@gm...>
>
> --- a/lib/chassis.c
> +++ b/lib/chassis.c
> @@ -362,13 +362,10 @@ chassis_mc_control_active_handler(ipmi_m
>      ipmi_domain_t  *domain = ipmi_mc_get_domain(mc);
>
>      if (active)
>    return;
>
> -    ipmi_mc_remove_active_handler(mc,
> -             chassis_mc_control_active_handler,
> -             control);
>   
This doesn't really hurt anything, I guess, though it may result in a 
memory leak.  I think it's just there for consistency.

>      _ipmi_domain_entity_lock(domain);
>      _ipmi_entity_get(entity);
>      _ipmi_domain_entity_unlock(domain);
>      ipmi_control_destroy(control);
>      _ipmi_entity_put(entity);
> --- a/lib/mc.c
> +++ b/lib/mc.c
> @@ -1382,12 +1382,12 @@ mc_reread_sel_timeout(void *cb_data, os_
>       lock, so just don't start the timer and everything should
>       be happy. */
>    DEBUG_INFO(info);
>    info->processing = 0;
>    info->timer_running = 0;
> -  ipmi_unlock(info->lock);
>      }
> +  ipmi_unlock(info->lock);
>  }
>   
This is very bizarre.  Read the code, this is clearly wrong.  The 
callback unlocks the lock, you only unlock it here if the callback fails.