[libvirt] [PATCH 1/2] qemu: avoid dead store in doPeer2PeerMigrate3
* src/qemu/qemu_migration.c: avoid dead 'ret' assignment and silence clang warning. Detected by ccc-analyzer: CC libvirt_driver_qemu_la-qemu_migration.lo qemu/qemu_migration.c:2046:5: warning: Value stored to 'ret' is never read ret = qemuMigrationConfirm(driver, sconn, vm, ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ --- src/qemu/qemu_migration.c | 3 +++ 1 files changed, 3 insertions(+), 0 deletions(-) diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index 5510493..469bb4a 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -2050,6 +2050,9 @@ finish: * do, but fortunately worst case is that there is a * domain left in 'paused' state on source. */ + if (ret < 0) + VIR_WARN("Guest %s probably left in 'paused' state on source", + vm->def->name); cleanup: if (ddomain) { -- 1.7.1
* src/qemu/qemu_migration.c: avoid dead 'ret' assignment and silence clang warning. Detected by ccc-analyzer: libvirt.c:4277:5: warning: Value stored to 'ret' is never read ret = domain->conn->driver->domainMigrateConfirm3 ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ --- src/libvirt.c | 3 +++ 1 files changed, 3 insertions(+), 0 deletions(-) diff --git a/src/libvirt.c b/src/libvirt.c index c8af3e1..023e9a7 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -4281,6 +4281,9 @@ confirm: * do, but fortunately worst case is that there is a * domain left in 'paused' state on source. */ + if (ret < 0) + VIR_WARN("Guest %s probably left in 'paused' state on source", + domain->name); done: if (orig_err) { -- 1.7.1
于 2011年08月16日 17:24, Alex Jia 写道:
* src/qemu/qemu_migration.c: avoid dead 'ret' assignment and silence clang warning.
Detected by ccc-analyzer:
CC libvirt_driver_qemu_la-qemu_migration.lo qemu/qemu_migration.c:2046:5: warning: Value stored to 'ret' is never read ret = qemuMigrationConfirm(driver, sconn, vm, ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
--- src/qemu/qemu_migration.c | 3 +++ 1 files changed, 3 insertions(+), 0 deletions(-)
diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index 5510493..469bb4a 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -2050,6 +2050,9 @@ finish: * do, but fortunately worst case is that there is a * domain left in 'paused' state on source. */ + if (ret< 0) + VIR_WARN("Guest %s probably left in 'paused' state on source", + vm->def->name);
Error is already reported by qemuMigrationConfirm if it fails on resuming the guest on source host. Think it's not worth to introduce a warning just to avoid the clang warning.
cleanup: if (ddomain) {
On 08/16/2011 06:02 PM, Osier Yang wrote:
于 2011年08月16日 17:24, Alex Jia 写道:
* src/qemu/qemu_migration.c: avoid dead 'ret' assignment and silence clang warning.
Detected by ccc-analyzer:
CC libvirt_driver_qemu_la-qemu_migration.lo qemu/qemu_migration.c:2046:5: warning: Value stored to 'ret' is never read ret = qemuMigrationConfirm(driver, sconn, vm, ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
--- src/qemu/qemu_migration.c | 3 +++ 1 files changed, 3 insertions(+), 0 deletions(-)
diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index 5510493..469bb4a 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -2050,6 +2050,9 @@ finish: * do, but fortunately worst case is that there is a * domain left in 'paused' state on source. */ + if (ret< 0) + VIR_WARN("Guest %s probably left in 'paused' state on source", + vm->def->name);
Error is already reported by qemuMigrationConfirm if it fails on resuming the guest on source host. Think it's not worth to introduce a warning just to avoid the clang warning. We should probably emit a log message if confirm failed, at which point, ret would be used in the conditional deciding whether to do a log message rather than value stored to 'ret' is never read, I have ever discussed this issue with Eric, this is his advice.
Thanks, Alex
cleanup: if (ddomain) {
On 08/16/2011 04:02 AM, Osier Yang wrote:
+++ b/src/qemu/qemu_migration.c @@ -2050,6 +2050,9 @@ finish: * do, but fortunately worst case is that there is a * domain left in 'paused' state on source. */ + if (ret< 0) + VIR_WARN("Guest %s probably left in 'paused' state on source", + vm->def->name);
Error is already reported by qemuMigrationConfirm if it fails on resuming the guest on source host. Think it's not worth to introduce a warning just to avoid the clang warning.
The issue at hand here is that the overall migration is returning status 0 (success), even if confirm failed. Are we sure that the qemuReportError in qemuMigrationConfirm always shows up in the logs? We don't need a double log entry; and if the confirm failure is already being logged, then the appropriate fix here is to just nuke the dead assignment (that is, call the confirm command without capturing its status into ret). But if the qemuReportError is not showing up in the log because the overall migration command is succeeding, then adding a VIR_WARN here to guarantee a log message is appropriate. I haven't researched the code well enough to know which solution is needed, but it should be the same solution for both libvirt.c and qemu_migration.c, since both files reported the same dead store under clang. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On 08/16/2011 03:24 AM, Alex Jia wrote:
* src/qemu/qemu_migration.c: avoid dead 'ret' assignment and silence clang warning.
Detected by ccc-analyzer:
CC libvirt_driver_qemu_la-qemu_migration.lo qemu/qemu_migration.c:2046:5: warning: Value stored to 'ret' is never read ret = qemuMigrationConfirm(driver, sconn, vm, ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
--- src/qemu/qemu_migration.c | 3 +++ 1 files changed, 3 insertions(+), 0 deletions(-)
diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index 5510493..469bb4a 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -2050,6 +2050,9 @@ finish: * do, but fortunately worst case is that there is a * domain left in 'paused' state on source. */ + if (ret< 0) + VIR_WARN("Guest %s probably left in 'paused' state on source", + vm->def->name);
The discussion on this died out, and I was left with the conclusion that if the overall migration returns success, then VIR_WARN is indeed the only way to log an error (using virterror.c only goes into the log on failures, not on success); meanwhile your patch does indeed silence the static analyzer. So: ACK, and both of your patches are now pushed. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
participants (4)
-
ajia＠redhat.com -
Alex Jia
-
Eric Blake -
Osier Yang