Hi Peter, Thanks a lot for your review! I'm so sorry for the delay, please see my reply below. 在 2018/6/8 16:05, Peter Krempa 写道: > On Thu, Jun 07, 2018 at 15:09:58 +0800, Weilun Zhu wrote: >> As qemuMonitorJSONIOProcess() will unlock the qemu monitor, there is >> some extreme situation, eg qemu send message to monitor twice in a short >> time, where the local viriable 'msg' of qemuMonitorIOProcess() could be > > I'd write this as: > > As qemuMonitorJSONIOProcess will call qemuMonitorJSONIOProcessEvent > which unlocks the monitor mutex > All right, it's much better. >> a wild point: >> >> 1. qemuMonitorSend() assign mon->msg to parameter 'msg', which is alse a >> local variable of its caller qemuMonitorJSONCommandWithFd(), cause >> eventloop to send message to monitor, then wait condition. >> 2. qemu send message to monitor for the first time immediately. >> 3. qemuMonitorIOProcess() is called, then wake up the qemuMonitorSend() >> thread, but the qemuMonitorSend() thread stuck for a while as cpu pressure >> or some other reasons,, which means the qemu monitor is still unlocked. >> 4. qemu send event message to monitor for the second time, >> such as RTC_CHANGE event >> 5. qemuMonitorIOProcess() is called, the local viriable 'msg' is >> assigned to mon->msg. >> 6. qemuMonitorIOProcess() call qemuMonitorJSONIOProcess() to deal with >> the qemu event. >> 7. qemuMonitorJSONIOProcess() unlock the qemu monitor in the macro >> 'QEMU_MONITOR_CALLBACK', then qemuMonitorSend() thread get the mutex >> and free the mon->msg, assign mon->msg to NULL. > > This is okay > >> >> so the local viriable 'msg' of qemuMonitorIOProcess() is a wild pointer >> now. >> >> AFAIK, it is not harmful to call again virCondBroadcast() while msg is a >> wild pointer, but just in case, we fix it in this patch. > > These two paragraphs can be dropped. > > > Also we now require that the authors of patches agree to the > 'Developer's certificate of origin' ( https://developercertificate.org/ > ). You express your agreement by adding a 'Signed-off-by' line. Without > that, your patch can't be pushed. > > OK, I did not notice that, I will add it. >> --- >> src/qemu/qemu_monitor.c | 8 ++++++++ >> 1 file changed, 8 insertions(+) >> >> diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c >> index 215135aa3e..a4b8572e24 100644 >> --- a/src/qemu/qemu_monitor.c >> +++ b/src/qemu/qemu_monitor.c >> @@ -463,6 +463,14 @@ qemuMonitorIOProcess(qemuMonitorPtr mon) >> #if DEBUG_IO >> VIR_DEBUG("Process done %d used %d", (int)mon->bufferOffset, len); >> #endif >> + >> + /* As the monitor mutex was unlocked in qemuMonitorJSONIOProcess() >> + * while dealing with qemu event, mon->msg could be changed, >> + * thus we re-acquire the msg here */ >> + msg = NULL; > > This part is okay (except for the last line if you apply what I suggest > bellow) > >> + if (mon->msg && mon->msg->txOffset == mon->msg->txLength) { >> + msg = mon->msg; > > But this condition can be merged with the one below. > >> + >> if (msg && msg->finished) > > It should look like: > if (mon->msg && mon->msg->finished) > virCondBroadcast(&mon->notify); > > The part with the txOffset is not necessary any more, since > msg->finished will be set only when that was true. The main reasoning is > that we don't really need to extract msg at this point any more. > > OK, your suggest is better, and I will change the comment like following: " - if (msg && msg->finished) + + /* As the monitor mutex was unlocked in qemuMonitorJSONIOProcess() + * while dealing with qemu event, mon->msg could be changed which + * means the above 'msg' may be invalid, thus we use 'mon->msg' here */ + if (mon->msg && mon->msg->finished) " >> virCondBroadcast(&mon->notify); >> return len; >> -- >> 2.18.0.rc1 >> >> -- >> libvir-list mailing list >> libvir-list@xxxxxxxxxx >> https://www.redhat.com/mailman/listinfo/libvir-list -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
