[libvirt] [PATCH 0/3] Fix coverity issues introduced by qemuagenttest and the refactor
Fix the issues reported by John and also add yet another test subcase. Peter Krempa (3): qemuagenttest: Fix checking of shutdown mode qemuagenttest: Check invalid response in shutdown test qemumonitortestutils: Don't skip va_end() on error path tests/qemuagenttest.c | 32 +++++++++++++++++++++++++++++++- tests/qemumonitortestutils.c | 2 +- 2 files changed, 32 insertions(+), 2 deletions(-) -- 1.8.3.2
Coverity complained about unused variable that contains the shutdown mode. The original intention was to check it against the requested mode. Also the fixed check reveald a mistake in the expected shutdown mode. Reported by John Ferlan. --- tests/qemuagenttest.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/tests/qemuagenttest.c b/tests/qemuagenttest.c index 4b6392b..e0df297 100644 --- a/tests/qemuagenttest.c +++ b/tests/qemuagenttest.c @@ -251,6 +251,13 @@ qemuAgentShutdownTestMonitorHandler(qemuMonitorTestPtr test, goto cleanup; } + if (STRNEQ(mode, data->mode)) { + ret = qemuMonitorReportError(test, + "expected shutdown mode '%s' got '%s'", + data->mode, mode); + goto cleanup; + } + /* now don't reply but return a qemu agent event */ qemuAgentNotifyEvent(qemuMonitorTestGetAgent(test), data->event); @@ -279,7 +286,7 @@ testQemuAgentShutdown(const void *data) goto cleanup; priv.event = QEMU_AGENT_EVENT_SHUTDOWN; - priv.mode = "shutdown"; + priv.mode = "halt"; if (qemuMonitorTestAddHandler(test, qemuAgentShutdownTestMonitorHandler, &priv, NULL) < 0) -- 1.8.3.2
On 08/01/2013 10:39 AM, Peter Krempa wrote:
Coverity complained about unused variable that contains the shutdown mode. The original intention was to check it against the requested mode.
Also the fixed check reveald a mistake in the expected shutdown mode.
Reported by John Ferlan. --- tests/qemuagenttest.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-)
ACK John
On 08/01/2013 08:39 AM, Peter Krempa wrote:
Coverity complained about unused variable that contains the shutdown mode. The original intention was to check it against the requested mode.
Also the fixed check reveald a mistake in the expected shutdown mode.
s/reveald/revealed/
Reported by John Ferlan. --- tests/qemuagenttest.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-)
-- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
The shutdown test utilizes waiting for condition to exit the test. This addition will return an error for the shutdown command to see if the condition waiting code will not hang. --- tests/qemuagenttest.c | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/tests/qemuagenttest.c b/tests/qemuagenttest.c index e0df297..f889b93 100644 --- a/tests/qemuagenttest.c +++ b/tests/qemuagenttest.c @@ -324,6 +324,29 @@ testQemuAgentShutdown(const void *data) QEMU_AGENT_SHUTDOWN_REBOOT) < 0) goto cleanup; + /* check negative response, so that we can verify that the agent breaks + * out from sleep */ + + if (qemuMonitorTestAddAgentSyncResponse(test) < 0) + goto cleanup; + + if (qemuMonitorTestAddItem(test, "guest-shutdown", + "{\"error\":" + " {\"class\":\"CommandDisabled\"," + " \"desc\":\"The command guest-shutdown has " + "been disabled for this instance\"," + " \"data\":{\"name\":\"guest-shutdown\"}" + " }" + "}") < 0) + goto cleanup; + + if (qemuAgentShutdown(qemuMonitorTestGetAgent(test), + QEMU_AGENT_SHUTDOWN_REBOOT) != -1) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + "agent shutdown command should have failed"); + goto cleanup; + } + ret = 0; cleanup: -- 1.8.3.2
On 08/01/2013 08:39 AM, Peter Krempa wrote:
The shutdown test utilizes waiting for condition to exit the test. This addition will return an error for the shutdown command to see if the condition waiting code will not hang. --- tests/qemuagenttest.c | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+)
ACK - negative tests are important too :) -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 08/01/2013 10:39 AM, Peter Krempa wrote:
The shutdown test utilizes waiting for condition to exit the test. This addition will return an error for the shutdown command to see if the condition waiting code will not hang. --- tests/qemuagenttest.c | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+)
ACK (although running with VIR_TEST_DEBUG=1 results in a message being printed which threw me at first). John
On 08/01/2013 11:57 AM, John Ferlan wrote:
On 08/01/2013 10:39 AM, Peter Krempa wrote:
The shutdown test utilizes waiting for condition to exit the test. This addition will return an error for the shutdown command to see if the condition waiting code will not hang. --- tests/qemuagenttest.c | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+)
ACK (although running with VIR_TEST_DEBUG=1 results in a message being printed which threw me at first).
Do we need to call a reset function to clear the expected error? -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 08/01/13 20:01, Eric Blake wrote:
On 08/01/2013 11:57 AM, John Ferlan wrote:
On 08/01/2013 10:39 AM, Peter Krempa wrote:
The shutdown test utilizes waiting for condition to exit the test. This addition will return an error for the shutdown command to see if the condition waiting code will not hang. --- tests/qemuagenttest.c | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+)
ACK (although running with VIR_TEST_DEBUG=1 results in a message being printed which threw me at first).
Do we need to call a reset function to clear the expected error?
This will probably warrant a separate refactor of the tests. There are a few of those that already report an (expected) error and then end successfully. Peter
When allocation of memory failed the error path didn't call va_end() causing a coverity failure. Reported by John Ferlan. --- tests/qemumonitortestutils.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/qemumonitortestutils.c b/tests/qemumonitortestutils.c index fb76156..5697946 100644 --- a/tests/qemumonitortestutils.c +++ b/tests/qemumonitortestutils.c @@ -669,7 +669,7 @@ qemuMonitorTestAddItemParams(qemuMonitorTestPtr test, va_start(args, response); if (VIR_ALLOC(data) < 0) - return -1; + goto error; if (VIR_STRDUP(data->command_name, cmdname) < 0 || VIR_STRDUP(data->response, response) < 0) -- 1.8.3.2
On 08/01/2013 08:39 AM, Peter Krempa wrote:
When allocation of memory failed the error path didn't call va_end() causing a coverity failure.
Reported by John Ferlan. --- tests/qemumonitortestutils.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
ACK.
diff --git a/tests/qemumonitortestutils.c b/tests/qemumonitortestutils.c index fb76156..5697946 100644 --- a/tests/qemumonitortestutils.c +++ b/tests/qemumonitortestutils.c @@ -669,7 +669,7 @@ qemuMonitorTestAddItemParams(qemuMonitorTestPtr test, va_start(args, response);
if (VIR_ALLOC(data) < 0) - return -1; + goto error;
if (VIR_STRDUP(data->command_name, cmdname) < 0 || VIR_STRDUP(data->response, response) < 0)
-- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 08/01/2013 10:39 AM, Peter Krempa wrote:
When allocation of memory failed the error path didn't call va_end() causing a coverity failure.
Reported by John Ferlan. --- tests/qemumonitortestutils.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
ACK John
On 08/01/13 19:57, John Ferlan wrote:
On 08/01/2013 10:39 AM, Peter Krempa wrote:
When allocation of memory failed the error path didn't call va_end() causing a coverity failure.
Reported by John Ferlan. --- tests/qemumonitortestutils.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
ACK
John
Series pushed. Thanks for the reviews :) Peter
participants (3)
-
Eric Blake -
John Ferlan -
Peter Krempa