<div dir="ltr"><div class="gmail-gE gmail-iv gmail-gt" style="padding:20px 0px 0px;font-size:0.875rem;font-family:Roboto,RobotoDraft,Helvetica,Arial,sans-serif"><br class="gmail-Apple-interchange-newline"><table cellpadding="0" class="gmail-cf gmail-gJ" style="border-collapse:collapse;margin-top:0px;width:auto;font-size:0.875rem;letter-spacing:0.2px;display:block"><tbody style="display:block"><tr class="gmail-acZ" style="height:auto;display:flex"><td class="gmail-gF gmail-gK" style="white-space:nowrap;padding:0px;vertical-align:top;width:1277.98px;line-height:20px;display:block;max-height:20px"><table cellpadding="0" class="gmail-cf gmail-ix" style="border-collapse:collapse;table-layout:fixed;width:1277px"><tbody><tr><td class="gmail-c2" style="display:flex"><h3 class="gmail-iw" style="overflow:hidden;font-size:0.75rem;font-weight:inherit;margin:inherit;text-overflow:ellipsis;letter-spacing:0.3px;color:rgb(95,99,104);line-height:20px"><span class="gmail-qu" tabindex="-1"><span name="Michal Privoznik" class="gmail-gD" style="color:rgb(32,33,36);font-size:0.875rem;font-weight:bold;display:inline;vertical-align:top;letter-spacing:0.2px;line-height:20px">Michal Privoznik</span></span></h3></td></tr></tbody></table></td><td class="gmail-gH gmail-bAk" style="text-align:right;white-space:nowrap;vertical-align:top;display:block;max-height:20px"><div class="gmail-gK" style="padding:0px;display:flex"><span id="gmail-:n5" class="gmail-g3" title="2019年11月28日 下午5:58" alt="2019年11月28日 下午5:58" tabindex="-1" style="vertical-align:top;margin:0px;font-size:0.75rem;letter-spacing:0.3px;color:rgb(95,99,104);display:block;line-height:20px">下午5:58 (1小时前)</span><div class="gmail-zd gmail-bi4" title="未加星标" tabindex="0" style="display:inline-block;height:20px;margin-left:20px;outline:0px"><span class="gmail-T-KT" style="display:inline-flex;height:20px;text-align:center;width:20px;padding:0px;margin:0px;border:none;outline:none"></span></div></div></td><td class="gmail-gH" style="text-align:right;white-space:nowrap;vertical-align:top;display:flex"></td><td class="gmail-gH gmail-acX gmail-bAm" rowspan="2" style="text-align:right;white-space:nowrap;vertical-align:top;display:block;max-height:20px"><div class="gmail-T-I gmail-J-J5-Ji gmail-T-I-Js-IF gmail-aaq gmail-T-I-ax7 gmail-L3" tabindex="0" style="display:inline-flex;border-radius:2px 0px 0px 2px;font-size:0.875rem;text-align:center;margin:0px 0px 0px 20px;height:20px;line-height:18px;min-width:0px;outline:none;padding:0px;background:transparent;color:rgb(68,68,68);border:none"><img class="gmail-hB gmail-T-I-J3" src="https://mail.google.com/mail/u/0/images/cleardot.gif" alt="" style="background: url("https://www.gstatic.com/images/icons/material/system/1x/reply_black_20dp.png") 50% 50% / 20px no-repeat; height: 20px; margin: 0px; vertical-align: middle; width: 20px; opacity: 0.54; display: inline-block; padding: 0px; transition: opacity 0.15s cubic-bezier(0.4, 0, 0.2, 1) 0s;"></div><div id="gmail-:ng" class="gmail-T-I gmail-J-J5-Ji gmail-T-I-Js-Gs gmail-aap gmail-T-I-awG gmail-T-I-ax7 gmail-L3" tabindex="0" style="display:inline-flex;border-radius:0px 2px 2px 0px;font-size:0.875rem;text-align:center;margin:0px 0px 0px 20px;height:20px;line-height:18px;min-width:0px;outline:none;padding:0px;background:transparent;color:rgb(68,68,68);border:none"><img class="gmail-hA gmail-T-I-J3" src="https://mail.google.com/mail/u/0/images/cleardot.gif" alt="" style="background: url("https://www.gstatic.com/images/icons/material/system/1x/more_vert_black_20dp.png") 50% 50% / 20px no-repeat; height: 20px; width: 20px; margin: 0px; vertical-align: middle; opacity: 0.54; display: inline-block; padding: 0px; transition: opacity 0.15s cubic-bezier(0.4, 0, 0.2, 1) 0s;"></div></td></tr><tr class="gmail-acZ gmail-xD" style="height:auto;display:flex"><td colspan="3"><table cellpadding="0" class="gmail-cf gmail-adz" style="border-collapse:collapse;table-layout:fixed;white-space:nowrap;width:1503px"><tbody><tr><td class="gmail-ady" style="overflow:visible;text-overflow:ellipsis;display:flex;line-height:20px"><div class="gmail-iw gmail-ajw" style="overflow:hidden;max-width:92%;display:inline-block"><span class="gmail-hb" style="vertical-align:top;color:rgb(95,99,104);font-size:0.75rem;letter-spacing:0.3px;line-height:20px">发送至 <span dir="ltr" name="我" class="gmail-g2" style="vertical-align:top">我</span>、 <span dir="ltr" name="libvir-list" class="gmail-g2" style="vertical-align:top">libvir-list</span></span></div><div id="gmail-:nf" class="gmail-ajy" tabindex="0" style="display:inline-flex;margin-left:4px;vertical-align:top;border:none;outline:none"><img class="gmail-ajz" src="https://mail.google.com/mail/u/0/images/cleardot.gif" alt="" style="background: url("https://www.gstatic.com/images/icons/material/system/1x/arrow_drop_down_black_20dp.png") 50% 50% / 20px no-repeat; cursor: pointer; padding: 0px; vertical-align: baseline; height: 20px; width: 20px; border: none; margin: 0px 0px 0px auto; right: 0px; top: 0px; display: flex; opacity: 0.54;"></div></td></tr></tbody></table></td></tr></tbody></table></div><div id="gmail-:n1" style="font-family:Roboto,RobotoDraft,Helvetica,Arial,sans-serif;font-size:medium"><div class="gmail-qQVYZb"></div><div class="gmail-utdU2e"></div><div class="gmail-btm"></div></div><div class="gmail-" style=""><div class="gmail-aHl" style="font-family:Roboto,RobotoDraft,Helvetica,Arial,sans-serif;font-size:medium"></div><div id="gmail-:ne" tabindex="-1" style="font-family:Roboto,RobotoDraft,Helvetica,Arial,sans-serif;font-size:medium"></div><div id="gmail-:n3" class="gmail-ii gmail-gt gmail-adO" style="direction:ltr;margin:8px 0px 0px;padding:0px"><div id="gmail-:n2" class="gmail-a3s gmail-aXjCH" style="font-family:Arial,Helvetica,sans-serif;font-size:small;overflow:hidden;font-variant-numeric:normal;font-variant-east-asian:normal;font-stretch:normal;line-height:1.5"><span class="gmail-im" style="color:rgb(80,0,80)">On 11/28/19 4:09 AM, Lance Liu wrote:<br>> Hi, Michal:<br>> I think the daemonFreeClientStream(NULL, stream); in your patch <br>> should line above virMutexUnlock(&stream->priv->lock)， or else there <br>> is issue WAW.<br><br></span>I'm sorry, I don't know what WAW is.<span class="gmail-im" style=""><br><br><font color="#0000ff">write after write,<b> </b></font><font color="#0000ff" style="">daemonFreeClientStream() outside stream->priv->lock</font></span></div><div id="gmail-:n2" class="gmail-a3s gmail-aXjCH" style="font-family:Arial,Helvetica,sans-serif;font-size:small;overflow:hidden;font-variant-numeric:normal;font-variant-east-asian:normal;font-stretch:normal;line-height:1.5"><span class="gmail-im" style=""><font color="#0000ff">may lead to two threads call daemonFreeClientStream() simultaneously</font></span></div><div id="gmail-:n2" class="gmail-a3s gmail-aXjCH" style="font-family:Arial,Helvetica,sans-serif;font-size:small;overflow:hidden;font-variant-numeric:normal;font-variant-east-asian:normal;font-stretch:normal;line-height:1.5"><span class="gmail-im" style=""><font color="#0000ff"><br></font><font color="#500050">> That is two threads may sub stream-></font><br><font color="#500050">> </font><br></span>> Lance Liu <<a href="mailto:liu.lance.89@gmail.com" target="_blank">liu.lance.89@gmail.com</a> <mailto:<a href="mailto:liu.lance.89@gmail.com" target="_blank">liu.lance.89@gmail.com</a>>> 于 <br><span class="gmail-im" style="color:rgb(80,0,80)">> 2019年11月26日周二 下午1:54写道：<br>> <br>> Yeah, your patch is perfectly fine for stream freed problem.<br>> But I have reviewed code in daemonStreamEvent() and<br>> daemonStreamFilter() again, and I think there still two issue need<br>> to be reconsidered:<br>> 1. stream->ref only ++ in daemonStreamFilter, except for error<br>> occurred call daemonRemoveClientStream() lead to ref--, like code as<br>> follow. Though code won't be executed because of<br>> no VIR_STREAM_EVENT_HANGUP event taken place,<br>> but it is not good code:<br><br></span>Do you perhaps mean daemonStreamEvent() instead of daemonStreamFilter()? <br>Because in daemonStreamFilter() we do both ref & unref - the unref is <br>hidden in daemonFreeClientStream().<div><div class="gmail-adm" style="margin:5px 0px"><div id="gmail-q_97" class="gmail-ajR gmail-h4" style="background-color:rgb(232,234,237);border:none;clear:both;line-height:6px;outline:none;width:24px;color:rgb(80,0,80);font-size:11px;border-radius:5.5px"><div class="gmail-ajT" style="background:url("https://www.gstatic.com/images/icons/material/system/1x/more_horiz_black_20dp.png") 50% 50%/20px no-repeat;height:11px;opacity:0.54;width:24px"></div></div></div></div>Again, the unref is hidden in daemonStreamMessageFinished() which calls <br>daemonFreeClientStream(). The msg->cb is guaranteed to be called <br>eventually upon successful return from <br>virNetServerProgramSendStreamData(). That's why you don't see the direct <br>unref in the success path.<span class="gmail-im" style=""><br><br><font color="#0000ff">Yeah, got it. I missed that!</font></span></div><div id="gmail-:n2" class="gmail-a3s gmail-aXjCH" style="font-family:Arial,Helvetica,sans-serif;font-size:small;overflow:hidden;font-variant-numeric:normal;font-variant-east-asian:normal;font-stretch:normal;line-height:1.5"><span class="gmail-im" style=""><font color="#500050">> }</font><br><font color="#500050">> 2. call virNetServerClientClose() is still inappropriate in because</font><br><font color="#500050">> the it may free the client resource which other threads need ref,</font><br><font color="#500050">> may be replace it with virNetServerClientImmedia</font><font color="#500050">teClose() is better,</font><br><font color="#500050">> the daemon can process client resource release unified</font><br><br></span>I've seen the patch you send, but it was without any commit message so <br>no one had any idea why the change is needed.<br>I understand your argument here, but I'm having hard time finding <br>practical example in the code. I mean, have you seen such issue in real?<span class="gmail-im" style="color:rgb(80,0,80)"><br><br>> 3. Segmentfault may still exists when another thread<br>> call remoteClientCloseFunc in virNetServerClientClose()(for example,<br>> when new force console session break down existed console session,<br>> and existed console session<br>> 's client close the session simutaneously, but remoteClientCloseFunc<br>> not lock(priv->lock), so the resource maybe released when<br>> daemonStreamEvent ref)<br><br></span>Again, sounds plausible, but this is a subset of the previous problem <br>since remoteClientCloseFunc() is called from <br>virNetServerClientCloseLocked() which is called from <br>virNetServerClientClose().</div><div id="gmail-:n2" class="gmail-a3s gmail-aXjCH" style="font-family:Arial,Helvetica,sans-serif;font-size:small;overflow:hidden;font-variant-numeric:normal;font-variant-east-asian:normal;font-stretch:normal;line-height:1.5"><br></div><div id="gmail-:n2" class="gmail-a3s gmail-aXjCH" style="font-family:Arial,Helvetica,sans-serif;font-size:small;overflow:hidden;font-variant-numeric:normal;font-variant-east-asian:normal;font-stretch:normal;line-height:1.5"><font color="#0000ff">I'll show you how to get the bug。Use gdb attach the libvirt daemon</font></div><div id="gmail-:n2" class="gmail-a3s gmail-aXjCH" style="font-family:Arial,Helvetica,sans-serif;font-size:small;overflow:hidden;font-variant-numeric:normal;font-variant-east-asian:normal;font-stretch:normal;line-height:1.5"><font color="#0000ff">and break in the middle of </font><span style="font-size:0.875rem;font-family:Roboto,RobotoDraft,Helvetica,Arial,sans-serif"><font color="#0000ff">daemonStreamEvent()(with event == 4,</font></span></div><div id="gmail-:n2" class="gmail-a3s gmail-aXjCH" style="font-family:Arial,Helvetica,sans-serif;font-size:small;overflow:hidden;font-variant-numeric:normal;font-variant-east-asian:normal;font-stretch:normal;line-height:1.5"><span style="font-size:0.875rem;font-family:Roboto,RobotoDraft,Helvetica,Arial,sans-serif"><font color="#0000ff">called by new force console break down existed console session),</font></span></div><div id="gmail-:n2" class="gmail-a3s gmail-aXjCH" style="font-family:Arial,Helvetica,sans-serif;font-size:small;overflow:hidden;font-variant-numeric:normal;font-variant-east-asian:normal;font-stretch:normal;line-height:1.5"><span style="font-size:0.875rem;font-family:Roboto,RobotoDraft,Helvetica,Arial,sans-serif"><font color="#0000ff">then exit from the previous virsh console client, so libvirt daemon can</font></span></div><div id="gmail-:n2" class="gmail-a3s gmail-aXjCH" style="font-family:Arial,Helvetica,sans-serif;font-size:small;overflow:hidden;font-variant-numeric:normal;font-variant-east-asian:normal;font-stretch:normal;line-height:1.5"><span style="font-size:0.875rem;font-family:Roboto,RobotoDraft,Helvetica,Arial,sans-serif"><font color="#0000ff">percent console client connection fd closed, then call the virNetServerClientClose()</font></span></div><div id="gmail-:n2" class="gmail-a3s gmail-aXjCH" style="overflow:hidden;font-variant-numeric:normal;font-variant-east-asian:normal;font-stretch:normal;line-height:1.5"><font color="#0000ff" face="Roboto, RobotoDraft, Helvetica, Arial, sans-serif"><span style="font-size:14px">in main thread, because the callback remote </span></font><font color="#0000ff">remoteClientCloseFunc() downes not </font></div><div id="gmail-:n2" class="gmail-a3s gmail-aXjCH" style="overflow:hidden;font-variant-numeric:normal;font-variant-east-asian:normal;font-stretch:normal;line-height:1.5"><font color="#0000ff">lock priv->lock, so it will release stream directly, regardless anther thread reference it </font></div><div id="gmail-:n2" class="gmail-a3s gmail-aXjCH" style="overflow:hidden;font-variant-numeric:normal;font-variant-east-asian:normal;font-stretch:normal;line-height:1.5"><font color="#0000ff">in daemonStreamEvent()</font></div><div id="gmail-:n2" class="gmail-a3s gmail-aXjCH" style="overflow:hidden;font-variant-numeric:normal;font-variant-east-asian:normal;font-stretch:normal;line-height:1.5"><font color="#0000ff"><br></font></div><div id="gmail-:n2" class="gmail-a3s gmail-aXjCH" style="overflow:hidden;font-variant-numeric:normal;font-variant-east-asian:normal;font-stretch:normal;line-height:1.5"><font color="#0000ff"><br></font></div><div id="gmail-:n2" class="gmail-a3s gmail-aXjCH" style="font-family:Arial,Helvetica,sans-serif;font-size:small;overflow:hidden;font-variant-numeric:normal;font-variant-east-asian:normal;font-stretch:normal;line-height:1.5"><br></div></div></div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">Michal Privoznik <<a href="mailto:mprivozn@redhat.com">mprivozn@redhat.com</a>> 于2019年11月28日周四 下午5:58写道：<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">On 11/28/19 4:09 AM, Lance Liu wrote:<br>
> Hi, Michal:<br>
> I think the daemonFreeClientStream(NULL, stream); in your patch <br>
> should line above virMutexUnlock(&stream->priv->lock)， or else there <br>
> is issue WAW.<br>
<br>
I'm sorry, I don't know what WAW is.<br>
<br>
> That is two threads may sub stream-><br>
> <br>
> Lance Liu <<a href="mailto:liu.lance.89@gmail.com" target="_blank">liu.lance.89@gmail.com</a> <mailto:<a href="mailto:liu.lance.89@gmail.com" target="_blank">liu.lance.89@gmail.com</a>>> 于 <br>
> 2019年11月26日周二 下午1:54写道：<br>
> <br>
> Yeah, your patch is perfectly fine for stream freed problem.<br>
> But I have reviewed code in daemonStreamEvent() and<br>
> daemonStreamFilter() again, and I think there still two issue need<br>
> to be reconsidered:<br>
> 1. stream->ref only ++ in daemonStreamFilter, except for error<br>
> occurred call daemonRemoveClientStream() lead to ref--, like code as<br>
> follow. Though code won't be executed because of<br>
> no VIR_STREAM_EVENT_HANGUP event taken place,<br>
> but it is not good code:<br>
<br>
Do you perhaps mean daemonStreamEvent() instead of daemonStreamFilter()? <br>
Because in daemonStreamFilter() we do both ref & unref - the unref is <br>
hidden in daemonFreeClientStream().<br>
<br>
> /* If we got HANGUP, we need to only send an empty<br>
> * packet so the client sees an EOF and cleans up<br>
> */<br>
> if (!stream->closed && !stream->recvEOF &&<br>
> (events & VIR_STREAM_EVENT_HANGUP)) {<br>
> virNetMessagePtr msg;<br>
> events &= ~(VIR_STREAM_EVENT_HANGUP);<br>
> stream->tx = false;<br>
> stream->recvEOF = true;<br>
> if (!(msg = virNetMessageNew(false))) {<br>
> daemonRemoveClientStream(client, stream);<br>
> virNetServerClientClose(client);<br>
> goto cleanup;<br>
> }<br>
> msg->cb = daemonStreamMessageFinished;<br>
> msg->opaque = stream;<br>
> stream->refs++;<br>
> if (virNetServerProgramSendStreamData(stream->prog,<br>
> client,<br>
> msg,<br>
> stream->procedure,<br>
> stream->serial,<br>
> "", 0) < 0) {<br>
> virNetMessageFree(msg);<br>
> daemonRemoveClientStream(client, stream);<br>
> virNetServerClientClose(client);<br>
> goto cleanup;<br>
> }<br>
<br>
Again, the unref is hidden in daemonStreamMessageFinished() which calls <br>
daemonFreeClientStream(). The msg->cb is guaranteed to be called <br>
eventually upon successful return from <br>
virNetServerProgramSendStreamData(). That's why you don't see the direct <br>
unref in the success path.<br>
<br>
> }<br>
> 2. call virNetServerClientClose() is still inappropriate in because<br>
> the it may free the client resource which other threads need ref,<br>
> may be replace it with virNetServerClientImmediateClose() is better,<br>
> the daemon can process client resource release unified<br>
<br>
I've seen the patch you send, but it was without any commit message so <br>
no one had any idea why the change is needed.<br>
I understand your argument here, but I'm having hard time finding <br>
practical example in the code. I mean, have you seen such issue in real?<br>
<br>
> 3. Segmentfault may still exists when another thread<br>
> call remoteClientCloseFunc in virNetServerClientClose()(for example,<br>
> when new force console session break down existed console session,<br>
> and existed console session<br>
> 's client close the session simutaneously, but remoteClientCloseFunc<br>
> not lock(priv->lock), so the resource maybe released when<br>
> daemonStreamEvent ref)<br>
<br>
Again, sounds plausible, but this is a subset of the previous problem <br>
since remoteClientCloseFunc() is called from <br>
virNetServerClientCloseLocked() which is called from <br>
virNetServerClientClose().<br>
<br>
Michal<br>
<br>
</blockquote></div>