On Mon, Oct 23, 2023 at 07:30:04PM +0200, Thomas Huth wrote:
On 23/10/2023 19.11, Thomas Huth wrote:
On 23/10/2023 17.57, Peter Xu wrote:
On Mon, Oct 23, 2023 at 04:50:44PM +0200, Thomas Huth wrote:
No need for a new variable here, especially not for one that shadows
a variable from the beginning of the function scope. With this change
the code now successfully compiles with -Wshadow=local.
Signed-off-by: Thomas Huth <thuth@redhat.com>
---
migration/ram.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/migration/ram.c b/migration/ram.c
index 92769902bb..9de9e54fa9 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -3238,8 +3238,7 @@ static int ram_save_complete(QEMUFile *f,
void *opaque)
ram_flush_compressed_data(rs);
- int ret = rdma_registration_stop(f, RAM_CONTROL_FINISH);
- if (ret < 0) {
+ if (rdma_registration_stop(f, RAM_CONTROL_FINISH) < 0) {
We may need to rename "ret" to something else? qemu_file_set_error(),
right below, will reference the error returned.
qemu_file_set_error(f, ret); <-----------------
Oh, drat, right ... that's exactly one of the reasons why shadowing
variables is a bad idea ;-)
I'll redo a v2.
Actually, there is more fishy stuff in this function:
static int ram_save_complete(QEMUFile *f, void *opaque)
{
...
int ret = 0;
...
WITH_RCU_READ_LOCK_GUARD() {
...
ret = rdma_registration_start(f, RAM_CONTROL_FINISH);
if (ret < 0) {
qemu_file_set_error(f, ret);
### here we use the outer "ret" variable ###
[1]
}
...
while (true) {
int pages;
pages = ram_find_and_save_block(rs);
/* no more blocks to sent */
if (pages == 0) {
### here we break without touching "ret" (preserving the previous error) ###
break;
}
if (pages < 0) {
ret = pages;
### we only replace the outer "ret" in this break-case here
break;
}
}
...
int ret = rdma_registration_stop(f, RAM_CONTROL_FINISH);
### so while ret from rdma_registration_start() might be propageted
### below, the ret from rdma_registration_stop() is only local here?
if (ret < 0) {
qemu_file_set_error(f, ret);
[2]
}
}
if (ret < 0) {
### this might trigger by the "ret" from rdma_registration_start() but
### not by the one from rdma_registration_stop()? ... very weird...
return ret;
}
Looks like commit 48408174a7ec7 messed up with the return types pretty badly
... any suggestions what's the right way forward here? Should the return
value of rdma_registration_start() only be used for the
qemu_file_set_error(), too? Or should the return value of
rdma_registration_stop() be allowed to be used for the "return ret" at the
end, too?
Right that's indeed confusing, but it seems confusing too even before that
commit. AFAICT, we should "break" for both [1][2] above for any error
occured..