* [PATCH] qga: fix assert regression on guest-shutdown
@ 2020-06-04 9:44 Marc-André Lureau
2020-06-04 10:09 ` Philippe Mathieu-Daudé
2020-06-04 13:43 ` Christian Ehrhardt
0 siblings, 2 replies; 5+ messages in thread
From: Marc-André Lureau @ 2020-06-04 9:44 UTC (permalink / raw)
To: qemu-devel; +Cc: Marc-André Lureau, Michael Roth
Since commit 781f2b3d1e ("qga: process_event() simplification"),
send_response() is called unconditionally, but will assert when "rsp" is
NULL. This may happen with QCO_NO_SUCCESS_RESP commands, such as
"guest-shutdown".
Fixes: 781f2b3d1e5ef389b44016a897fd55e7a780bf35
Cc: Michael Roth <mdroth@linux.vnet.ibm.com>
Reported-by: Christian Ehrhardt <christian.ehrhardt@canonical.com>
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
qga/main.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/qga/main.c b/qga/main.c
index f0e454f28d3..3febf3b0fdf 100644
--- a/qga/main.c
+++ b/qga/main.c
@@ -531,7 +531,11 @@ static int send_response(GAState *s, const QDict *rsp)
QString *payload_qstr, *response_qstr;
GIOStatus status;
- g_assert(rsp && s->channel);
+ g_assert(s->channel);
+
+ if (!rsp) {
+ return 0;
+ }
payload_qstr = qobject_to_json(QOBJECT(rsp));
if (!payload_qstr) {
--
2.26.2.561.g07d8ea56f2
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [PATCH] qga: fix assert regression on guest-shutdown
2020-06-04 9:44 [PATCH] qga: fix assert regression on guest-shutdown Marc-André Lureau
@ 2020-06-04 10:09 ` Philippe Mathieu-Daudé
2020-06-04 13:43 ` Christian Ehrhardt
1 sibling, 0 replies; 5+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-06-04 10:09 UTC (permalink / raw)
To: Marc-André Lureau, qemu-devel; +Cc: Michael Roth
On 6/4/20 11:44 AM, Marc-André Lureau wrote:
> Since commit 781f2b3d1e ("qga: process_event() simplification"),
> send_response() is called unconditionally, but will assert when "rsp" is
> NULL. This may happen with QCO_NO_SUCCESS_RESP commands, such as
> "guest-shutdown".
>
> Fixes: 781f2b3d1e5ef389b44016a897fd55e7a780bf35
> Cc: Michael Roth <mdroth@linux.vnet.ibm.com>
> Reported-by: Christian Ehrhardt <christian.ehrhardt@canonical.com>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
> qga/main.c | 6 +++++-
> 1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/qga/main.c b/qga/main.c
> index f0e454f28d3..3febf3b0fdf 100644
> --- a/qga/main.c
> +++ b/qga/main.c
> @@ -531,7 +531,11 @@ static int send_response(GAState *s, const QDict *rsp)
> QString *payload_qstr, *response_qstr;
> GIOStatus status;
>
> - g_assert(rsp && s->channel);
> + g_assert(s->channel);
> +
> + if (!rsp) {
> + return 0;
> + }
Why not assert after the check?
Anyway:
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>
> payload_qstr = qobject_to_json(QOBJECT(rsp));
> if (!payload_qstr) {
>
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] qga: fix assert regression on guest-shutdown
2020-06-04 9:44 [PATCH] qga: fix assert regression on guest-shutdown Marc-André Lureau
2020-06-04 10:09 ` Philippe Mathieu-Daudé
@ 2020-06-04 13:43 ` Christian Ehrhardt
2020-06-09 11:15 ` Christian Ehrhardt
1 sibling, 1 reply; 5+ messages in thread
From: Christian Ehrhardt @ 2020-06-04 13:43 UTC (permalink / raw)
To: Marc-André Lureau; +Cc: qemu-devel, Michael Roth
[-- Attachment #1: Type: text/plain, Size: 1273 bytes --]
On Thu, Jun 4, 2020 at 11:46 AM Marc-André Lureau <
marcandre.lureau@redhat.com> wrote:
> Since commit 781f2b3d1e ("qga: process_event() simplification"),
> send_response() is called unconditionally, but will assert when "rsp" is
> NULL. This may happen with QCO_NO_SUCCESS_RESP commands, such as
> "guest-shutdown".
>
> Fixes: 781f2b3d1e5ef389b44016a897fd55e7a780bf35
> Cc: Michael Roth <mdroth@linux.vnet.ibm.com>
> Reported-by: Christian Ehrhardt <christian.ehrhardt@canonical.com>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
> qga/main.c | 6 +++++-
> 1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/qga/main.c b/qga/main.c
> index f0e454f28d3..3febf3b0fdf 100644
> --- a/qga/main.c
> +++ b/qga/main.c
> @@ -531,7 +531,11 @@ static int send_response(GAState *s, const QDict *rsp)
> QString *payload_qstr, *response_qstr;
> GIOStatus status;
>
> - g_assert(rsp && s->channel);
> + g_assert(s->channel);
> +
> + if (!rsp) {
> + return 0;
> + }
>
>
>
Thanks Marc-André,
LGTM and should fix the issues I was seeing.
Reviewed-by: Christian Ehrhardt <christian.ehrhardt@canonical.com>
--
Christian Ehrhardt
Staff Engineer, Ubuntu Server
Canonical Ltd
[-- Attachment #2: Type: text/html, Size: 2112 bytes --]
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] qga: fix assert regression on guest-shutdown
2020-06-04 13:43 ` Christian Ehrhardt
@ 2020-06-09 11:15 ` Christian Ehrhardt
2020-06-15 7:24 ` Christian Ehrhardt
0 siblings, 1 reply; 5+ messages in thread
From: Christian Ehrhardt @ 2020-06-09 11:15 UTC (permalink / raw)
To: Marc-André Lureau; +Cc: qemu-devel, Michael Roth
[-- Attachment #1: Type: text/plain, Size: 1551 bytes --]
On Thu, Jun 4, 2020 at 3:43 PM Christian Ehrhardt <
christian.ehrhardt@canonical.com> wrote:
>
>
> On Thu, Jun 4, 2020 at 11:46 AM Marc-André Lureau <
> marcandre.lureau@redhat.com> wrote:
>
>> Since commit 781f2b3d1e ("qga: process_event() simplification"),
>> send_response() is called unconditionally, but will assert when "rsp" is
>> NULL. This may happen with QCO_NO_SUCCESS_RESP commands, such as
>> "guest-shutdown".
>>
>> Fixes: 781f2b3d1e5ef389b44016a897fd55e7a780bf35
>> Cc: Michael Roth <mdroth@linux.vnet.ibm.com>
>> Reported-by: Christian Ehrhardt <christian.ehrhardt@canonical.com>
>> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>> ---
>> qga/main.c | 6 +++++-
>> 1 file changed, 5 insertions(+), 1 deletion(-)
>>
>> diff --git a/qga/main.c b/qga/main.c
>> index f0e454f28d3..3febf3b0fdf 100644
>> --- a/qga/main.c
>> +++ b/qga/main.c
>> @@ -531,7 +531,11 @@ static int send_response(GAState *s, const QDict
>> *rsp)
>> QString *payload_qstr, *response_qstr;
>> GIOStatus status;
>>
>> - g_assert(rsp && s->channel);
>> + g_assert(s->channel);
>> +
>> + if (!rsp) {
>> + return 0;
>> + }
>>
>>
>>
> Thanks Marc-André,
> LGTM and should fix the issues I was seeing.
>
> Reviewed-by: Christian Ehrhardt <christian.ehrhardt@canonical.com>
>
In the meantime I also got to test this against the initially reported
issue, LGTM as well (ran as no-change backport onto 4.2).
Tested-by: Christian Ehrhardt <christian.ehrhardt@canonical.com>
[-- Attachment #2: Type: text/html, Size: 2724 bytes --]
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] qga: fix assert regression on guest-shutdown
2020-06-09 11:15 ` Christian Ehrhardt
@ 2020-06-15 7:24 ` Christian Ehrhardt
0 siblings, 0 replies; 5+ messages in thread
From: Christian Ehrhardt @ 2020-06-15 7:24 UTC (permalink / raw)
To: Marc-André Lureau; +Cc: qemu-devel, Michael Roth
[-- Attachment #1: Type: text/plain, Size: 1889 bytes --]
On Tue, Jun 9, 2020 at 1:15 PM Christian Ehrhardt <
christian.ehrhardt@canonical.com> wrote:
>
>
> On Thu, Jun 4, 2020 at 3:43 PM Christian Ehrhardt <
> christian.ehrhardt@canonical.com> wrote:
>
>>
>>
>> On Thu, Jun 4, 2020 at 11:46 AM Marc-André Lureau <
>> marcandre.lureau@redhat.com> wrote:
>>
>>> Since commit 781f2b3d1e ("qga: process_event() simplification"),
>>> send_response() is called unconditionally, but will assert when "rsp" is
>>> NULL. This may happen with QCO_NO_SUCCESS_RESP commands, such as
>>> "guest-shutdown".
>>>
>>> Fixes: 781f2b3d1e5ef389b44016a897fd55e7a780bf35
>>> Cc: Michael Roth <mdroth@linux.vnet.ibm.com>
>>> Reported-by: Christian Ehrhardt <christian.ehrhardt@canonical.com>
>>> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>>> ---
>>> qga/main.c | 6 +++++-
>>> 1 file changed, 5 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/qga/main.c b/qga/main.c
>>> index f0e454f28d3..3febf3b0fdf 100644
>>> --- a/qga/main.c
>>> +++ b/qga/main.c
>>> @@ -531,7 +531,11 @@ static int send_response(GAState *s, const QDict
>>> *rsp)
>>> QString *payload_qstr, *response_qstr;
>>> GIOStatus status;
>>>
>>> - g_assert(rsp && s->channel);
>>> + g_assert(s->channel);
>>> +
>>> + if (!rsp) {
>>> + return 0;
>>> + }
>>>
>>>
>>>
>> Thanks Marc-André,
>> LGTM and should fix the issues I was seeing.
>>
>> Reviewed-by: Christian Ehrhardt <christian.ehrhardt@canonical.com>
>>
>
> In the meantime I also got to test this against the initially reported
> issue, LGTM as well (ran as no-change backport onto 4.2).
>
> Tested-by: Christian Ehrhardt <christian.ehrhardt@canonical.com>
>
This LGTM with 2*reviews 1*tested and 11 days on the list without any
negative feedback.
I just wanted to re-check if there is anything else left for this to be
committed?
[-- Attachment #2: Type: text/html, Size: 3348 bytes --]
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2020-06-15 7:25 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2020-06-04 9:44 [PATCH] qga: fix assert regression on guest-shutdown Marc-André Lureau
2020-06-04 10:09 ` Philippe Mathieu-Daudé
2020-06-04 13:43 ` Christian Ehrhardt
2020-06-09 11:15 ` Christian Ehrhardt
2020-06-15 7:24 ` Christian Ehrhardt
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).