<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
</head>
<body>
<div class="moz-cite-prefix">Michael,</div>
<div class="moz-cite-prefix"><br>
</div>
<div class="moz-cite-prefix">You're right, that's the best approach
; thanks for sharing it with me.</div>
<div class="moz-cite-prefix"><br>
</div>
<div class="moz-cite-prefix">Here is a PR :
<a class="moz-txt-link-freetext" href="https://github.com/openvehicles/Open-Vehicle-Monitoring-System-3/pull/777">https://github.com/openvehicles/Open-Vehicle-Monitoring-System-3/pull/777</a>
for review.</div>
<div class="moz-cite-prefix"><br>
</div>
<div class="moz-cite-prefix">(I also converted the second parameter
- metric name - but you can remove this commit if you prefer)<br>
</div>
<div class="moz-cite-prefix"><br>
</div>
<div class="moz-cite-prefix">Regards,<br>
</div>
<div class="moz-cite-prefix"><br>
</div>
<div class="moz-cite-prefix">Le 18/11/2022 à 14:51, Michael Balzer a
écrit :<br>
</div>
<blockquote type="cite"
cite="mid:d3d833f0-38a3-fc8a-461b-c5c3057d436e@expeedo.de">
<meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
Ludovic,<br>
<br>
there is an even better variant of your second approach, that's
changing MetricCallbackEntry.m_caller to a std::string.<br>
<br>
That's the same scheme EventCallbackEntry already uses, and
m_caller is only used to identiy the registrant, so that's
straight forward.<br>
<br>
Regards,<br>
Michael<br>
<br>
<br>
<div class="moz-cite-prefix">Am 18.11.22 um 01:49 schrieb Ludovic
LANGE:<br>
</div>
<blockquote type="cite"
cite="mid:7ec4cb59-a69c-d702-64fe-dd3c3fe7e502@lange.nom.fr">
<meta http-equiv="Content-Type" content="text/html;
charset=UTF-8">
<div class="moz-cite-prefix">Some news...</div>
<div class="moz-cite-prefix"><br>
</div>
<p>My issue was caused because of course I made a mistake with
the way of registering my metrics Listener.</p>
<p>I used the following code (see my previous post) in the
constructor:<br>
</p>
<p><font face="monospace"> +
MyMetrics.RegisterListener(IDTAG.c_str(), "*",
std::bind(&canlog::MetricListener, this, _1));</font></p>
<div class="moz-cite-prefix">and for the destructor:</div>
<div class="moz-cite-prefix"><font face="monospace"> +
MyMetrics.DeregisterListener(IDTAG.c_str());</font></div>
<div class="moz-cite-prefix"><font face="monospace"><br>
</font></div>
<div class="moz-cite-prefix">I had to use IDTAG because this
class has multiple instances.</div>
<div class="moz-cite-prefix"><br>
</div>
<div class="moz-cite-prefix">The issue is passing <font
face="monospace">IDTAG.c_str()</font> as a parameter ;
because<font face="monospace"> MyMetrics.RegisterListener</font>
stores the value of this <font face="monospace">const char* </font>parameter
and uses it 1) as an immutable parameter and 2) as a value.<br>
</div>
<div class="moz-cite-prefix"><br>
</div>
<div class="moz-cite-prefix">Of course this does not work with
my first way to call <font face="monospace">MyMetrics.RegisterListener</font>
:</div>
<div class="moz-cite-prefix">
<ul>
<li> the memory buffer returned by <font face="monospace">IDTAG.c_str()</font>
has no more existence after exiting the constructor, so
this lead to memory corruption.</li>
<li>when calling <font face="monospace">IDTAG.c_str()</font>
again in the destructor, there is absolutely no chance
that this <font face="monospace">c_str() </font>call
returns the same memory address as the call in the
constructor, so the value comparison will never work.<br>
</li>
</ul>
</div>
<div class="moz-cite-prefix"><br>
</div>
<div class="moz-cite-prefix">I rewrote the code this way by
adding a new member (<font face="monospace">m_idtag</font>) to
the class:</div>
<div class="moz-cite-prefix"><br>
</div>
<div class="moz-cite-prefix"><font face="monospace">@
vehicle/OVMS.V3/components/can/src/canlog.cpp:352 @
canlog::canlog(const char* type, std::string format,
canformat::canformat_serve_<br>
m_dropcount = 0;<br>
m_filtercount = 0;<br>
<br>
+ m_idtag = strdup(IDTAG.c_str());<br>
+<br>
using std::placeholders::_1;<br>
using std::placeholders::_2;<br>
MyEvents.RegisterEvent(IDTAG, "*",
std::bind(&canlog::EventListener, this, _1, _2));<br>
+ if (NULL != m_idtag)<br>
+ {<br>
+ MyMetrics.RegisterListener(m_idtag, "*",
std::bind(&canlog::MetricListener, this, _1));<br>
+ }<br>
+ else<br>
+ {<br>
+ ESP_LOGE(TAG,"Cannot allocate memory - not registering
metrics listener");<br>
+ }<br>
<br>
int queuesize = MyConfig.GetParamValueInt("can",
"log.queuesize",100);<br>
m_queue = xQueueCreate(queuesize,
sizeof(CAN_log_message_t));<br>
@ vehicle/OVMS.V3/components/can/src/canlog.cpp:374 @
canlog::canlog(const char* type, std::string format,
canformat::canformat_serve_<br>
canlog::~canlog()<br>
{<br>
MyEvents.DeregisterEvent(IDTAG);<br>
+ if (NULL != m_idtag)<br>
+ {<br>
+ MyMetrics.DeregisterListener(m_idtag);<br>
+ free((void*)m_idtag);<br>
+ }<br>
<br>
if (m_task)<br>
{</font></div>
<div class="moz-cite-prefix"><br>
</div>
<div class="moz-cite-prefix"><br>
</div>
<div class="moz-cite-prefix">Another option is to change <font
face="monospace">MyMetrics.RegisterListener</font> so that
it store *a copy* of the parameter, and does string comparison
instead of a value comparison:</div>
<div class="moz-cite-prefix"><br>
</div>
<div class="moz-cite-prefix"><font face="monospace">modified:
vehicle/OVMS.V3/main/ovms_metrics.cpp<br>
────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────<br>
@ vehicle/OVMS.V3/main/ovms_metrics.cpp:1019 @ void
OvmsMetrics::RegisterListener(const char* caller, const
char* name, MetricC<br>
return;<br>
}<br>
<br>
+ const char * caller_copy = (const char *)strdup(caller);<br>
+<br>
+ if (NULL == caller_copy)<br>
+ {<br>
+ ESP_LOGE(TAG, "Problem registering metric %s for caller
%s - insufficient memory",name,caller);<br>
+ return;<br>
+ }<br>
+<br>
MetricCallbackList *ml = k->second;<br>
- ml->push_back(new
MetricCallbackEntry(caller,callback));<br>
+ ml->push_back(new
MetricCallbackEntry(caller_copy,callback));<br>
}<br>
<br>
void OvmsMetrics::DeregisterListener(const char* caller)<br>
@ vehicle/OVMS.V3/main/ovms_metrics.cpp:1041 @ void
OvmsMetrics::DeregisterListener(const char* caller)<br>
while (itc!=ml->end())<br>
{<br>
MetricCallbackEntry* ec = *itc;<br>
- if (ec->m_caller == caller)<br>
+ if (strcmp(ec->m_caller, caller) == 0)<br>
{<br>
+ free((char *)ec->m_caller);<br>
itc = ml->erase(itc);<br>
delete ec;<br>
}</font></div>
<div class="moz-cite-prefix"><br>
</div>
<div class="moz-cite-prefix"><br>
</div>
<div class="moz-cite-prefix">I've tested both approaches and
they both solve my issue : no more crashes and memory
corruption.<br>
</div>
<div class="moz-cite-prefix"><br>
</div>
<div class="moz-cite-prefix">The second one is more "risky" (as
it potentially impacts everyone) but I think that it should
not cause much regression.</div>
<div class="moz-cite-prefix"><br>
</div>
<div class="moz-cite-prefix">Let me know what you think - should
I make a PR for this second approach ?<br>
</div>
<div class="moz-cite-prefix"><br>
</div>
<div class="moz-cite-prefix">Regards,<br>
</div>
<div class="moz-cite-prefix"><br>
</div>
</blockquote>
</blockquote>
<div id="grammalecte_menu_main_button_shadow_host" style="width:
0px; height: 0px;"></div>
</body>
</html>