qemu: Don't send any monitor commands afer SHUTDOWN event

[libvirt] [PATCH] qemu: Don't send any monitor commands afer SHUTDOWN event

Posted by Michal Privoznik 6 years, 3 months ago

After aeda1b8c56dc5 we tried to stop reporting I/O errors on
expected monitor HUP. We've achieved that by not setting
mon->lastError once we've received SHUTDOWN event. However, this
makes us to deadlock in case there's thread that enters the
monitor after the event is received. The problem is, we're no
longer setting mon->lastError and therefore qemuMonitorSend()
does not return early and continues execution until virCondWait()
(which will never wake up).

Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
---
 src/qemu/qemu_monitor.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
index 85c7d68a1..9f3e3eb14 100644
--- a/src/qemu/qemu_monitor.c
+++ b/src/qemu/qemu_monitor.c
@@ -1063,7 +1063,7 @@ qemuMonitorSend(qemuMonitorPtr mon,
 {
     int ret = -1;
 
-    /* Check whether qemu quit unexpectedly */
+    /* Check whether qemu quit unexpectedly, */
     if (mon->lastError.code != VIR_ERR_OK) {
         VIR_DEBUG("Attempt to send command while error is set %s",
                   NULLSTR(mon->lastError.message));
@@ -1071,6 +1071,12 @@ qemuMonitorSend(qemuMonitorPtr mon,
         return -1;
     }
 
+    /* or expectedly. */
+    if (mon->willhangup) {
+        VIR_DEBUG("Attempt to send command while domain is shutting down");
+        return -1;
+    }
+
     mon->msg = msg;
     qemuMonitorUpdateWatch(mon);
 
-- 
2.13.6

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] qemu: Don't send any monitor commands afer SHUTDOWN event

Posted by Ján Tomko 6 years, 3 months ago

On Fri, Jan 19, 2018 at 10:02:45AM +0100, Michal Privoznik wrote:
>After aeda1b8c56dc5 we tried to stop reporting I/O errors on
>expected monitor HUP. We've achieved that by not setting
>mon->lastError once we've received SHUTDOWN event. However, this
>makes us to deadlock in case there's thread that enters the
>monitor after the event is received. The problem is, we're no
>longer setting mon->lastError and therefore qemuMonitorSend()
>does not return early and continues execution until virCondWait()
>(which will never wake up).
>
>Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
>---
> src/qemu/qemu_monitor.c | 8 +++++++-
> 1 file changed, 7 insertions(+), 1 deletion(-)
>
>diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
>index 85c7d68a1..9f3e3eb14 100644
>--- a/src/qemu/qemu_monitor.c
>+++ b/src/qemu/qemu_monitor.c
>@@ -1063,7 +1063,7 @@ qemuMonitorSend(qemuMonitorPtr mon,
> {
>     int ret = -1;
>
>-    /* Check whether qemu quit unexpectedly */
>+    /* Check whether qemu quit unexpectedly, */
>     if (mon->lastError.code != VIR_ERR_OK) {
>         VIR_DEBUG("Attempt to send command while error is set %s",
>                   NULLSTR(mon->lastError.message));
>@@ -1071,6 +1071,12 @@ qemuMonitorSend(qemuMonitorPtr mon,
>         return -1;
>     }
>
>+    /* or expectedly. */
>+    if (mon->willhangup) {
>+        VIR_DEBUG("Attempt to send command while domain is shutting down");
>+        return -1;
>+    }
>+

Not reporting an error here is not desirable in all the cases.

Quietly failing to send a monitor command might be fine for the
GetAllDomainStats use case, but exiting an API called on a specific
domain without setting an error is not okay.

IMO the right fix of the deadlock is to revert the patch.

Jan

>     mon->msg = msg;
>     qemuMonitorUpdateWatch(mon);
>
>-- 
>2.13.6
>
>--
>libvir-list mailing list
>libvir-list@redhat.com
>https://www.redhat.com/mailman/listinfo/libvir-list
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] qemu: Don't send any monitor commands afer SHUTDOWN event

Posted by Michal Privoznik 6 years, 3 months ago

On 01/19/2018 10:55 AM, Ján Tomko wrote:
> On Fri, Jan 19, 2018 at 10:02:45AM +0100, Michal Privoznik wrote:
>> After aeda1b8c56dc5 we tried to stop reporting I/O errors on
>> expected monitor HUP. We've achieved that by not setting
>> mon->lastError once we've received SHUTDOWN event. However, this
>> makes us to deadlock in case there's thread that enters the
>> monitor after the event is received. The problem is, we're no
>> longer setting mon->lastError and therefore qemuMonitorSend()
>> does not return early and continues execution until virCondWait()
>> (which will never wake up).
>>
>> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
>> ---
>> src/qemu/qemu_monitor.c | 8 +++++++-
>> 1 file changed, 7 insertions(+), 1 deletion(-)
>>
>> diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
>> index 85c7d68a1..9f3e3eb14 100644
>> --- a/src/qemu/qemu_monitor.c
>> +++ b/src/qemu/qemu_monitor.c
>> @@ -1063,7 +1063,7 @@ qemuMonitorSend(qemuMonitorPtr mon,
>> {
>>     int ret = -1;
>>
>> -    /* Check whether qemu quit unexpectedly */
>> +    /* Check whether qemu quit unexpectedly, */
>>     if (mon->lastError.code != VIR_ERR_OK) {
>>         VIR_DEBUG("Attempt to send command while error is set %s",
>>                   NULLSTR(mon->lastError.message));
>> @@ -1071,6 +1071,12 @@ qemuMonitorSend(qemuMonitorPtr mon,
>>         return -1;
>>     }
>>
>> +    /* or expectedly. */
>> +    if (mon->willhangup) {
>> +        VIR_DEBUG("Attempt to send command while domain is shutting
>> down");
>> +        return -1;
>> +    }
>> +
> 
> Not reporting an error here is not desirable in all the cases.
> 
> Quietly failing to send a monitor command might be fine for the
> GetAllDomainStats use case, but exiting an API called on a specific
> domain without setting an error is not okay.
> 
> IMO the right fix of the deadlock is to revert the patch.

Agreed. Let me post such patch.

Michal

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list