block migration: fix return value mismatch
PATCH 1/1 block migration: fix return value mismatch
The function uses a ternary return value (<, >, == 0) defined as an int. The code in in this function uses int64_t types to collect ftell() return values and use their difference as the return value. Unfortunately, narrowing of integer types results in the disposal of the left-most bits that won't fit in the target type. Here, for values larger than 2GB, the resulting value will be randomly negative or positive, based on total number of blocks. The patch ensures that only +1, -1, or 0 are returned to properly report status.
diff -u -r a/block-migration.c b/block-migration.c
--- a/block-migration.c 2014-04-17 08:30:59.000000000 -0500
+++ b/block-migration.c 2014-11-10 12:39:10.727431187 -0600
@@ -628,6 +628,7 @@
{
int ret;
int64_t last_ftell = qemu_ftell(f);+ int64_t delta_ftell;
DPRINTF("Enter save live iterate submitted %d transferred %d\n",
block_mig_state.submitted, block_mig_state.transferred);
@@ -677,7 +678,8 @@
}
qemu_put_be64(f, BLK_MIG_FLAG_EOS);- return qemu_ftell(f) - last_ftell;
+ delta_ftell = qemu_ftell(f) - last_ftell;
+ return( (delta_ftell > 0) ? 1 : (delta_ftell < 0) ? -1 : 0 );
}
/* Called with iothread lock taken. */
Quoted 2 lines · Gary Hook
The function uses a ternary return value (<, >, == 0) defined as an int. The code in in this function uses
int64_ttypes to collectftell()return values and use their difference as the return value. Unfortunately, narrowing of integer types results in the disposal of the left-most bits that won't fit in the target type. Here, for values larger than 2GB, the resulting value will be randomly negative or positive, based on total number of blocks. The patch ensures that only +1, -1, or 0 are returned to properly report status.
Please wrap commit messages at around 70 characters ('git log' likes to indent, and people still like to use 80-column windows to read 'git log').
Quoted 5 lines
diff -u -r
a/block-migration.cb/block-migration.c
Your patch is missing a 'Signed-off-by' line, which is essential to getting it applied. It is also missing the typical '---' separator between commit message and patch body.
Review
Quoted 5 lines
---
a/block-migration.c2014-04-17 08:30:59.000000000 -0500+++
b/block-migration.c2014-11-10 12:39:10.727431187 -0600(quoted diff) +0
-0@@ -628,6 +628,7 @@
Your patch is mal-formed, with too many blank lines inserted by your mailer. We generally request that you use 'git send-email' rather than manually pasting your patch into an email, since git is more likely to send a correctly-formed message.
Also, it appears that you sent a 0/1 cover letter, but forgot to put this message as In-Reply-To that message. For a single patch, no cover letter is necessary; but for a patch series, it is important to properly thread your emails.
More details on proper patch submission can be found here:
http://wiki.qemu.org/Contribute/SubmitAPatch
At any rate, the patch looks reasonable, so please resend it as v2 with your Signed-off-by.
Signature · 2 lines
Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Quoted 12 lines · Eric Blake
Quoted 10 lines · Gary Hook
The function uses a ternary return value (<, >, == 0) defined as an int. The code in in this function uses
int64_ttypes to collectftell()return values and use their difference as the return value. Unfortunately, narrowing of integer types results in the disposal of the left-most bits that won't fit in the target type. Here, for values larger than 2GB, the resulting value will be randomly negative or positive, based on total number of blocks. The patch ensures that only +1, -1, or 0 are returned to properly report status.
Quoted 2 lines
Please wrap commit messages at around 70 characters ('git log' likes to indent, and people still like to use 80-column windows to read 'git log').
The true reason for limiting commit message width is not humoring curmudgeons insisting on antiquated terminal widths, it's us antiquated human beings: we tend to have trouble following long lines with our eyes (I sure do). Typographic manuals suggest to limit columns to roughly 60 characters for exactly that reason[*]. 70 is already a compromise between legibility and "writability".
[...]
[*] https://en.wikipedia.org/wiki/Column_(typography)#Typographic_style
Review
Quoted 3 lines · Gary Hook
(quoted diff) +2
-1- return qemu_ftell(f) - last_ftell; + delta_ftell = qemu_ftell(f) - last_ftell; + return( (delta_ftell > 0) ? 1 : (delta_ftell < 0) ? -1 : 0 );
Good find!
Please don't nest the ternary operator, it is hard to read.
if (delta_ftell < 0) {
return -1;
} else if (delta_ftell > 0) {
return 1;
} else {
return 0;
}Quoted 9 lines · Stefan Hajnoczi
Review
Quoted 3 lines · Gary Hook
(quoted diff) +2
-1- return qemu_ftell(f) - last_ftell; + delta_ftell = qemu_ftell(f) - last_ftell; + return( (delta_ftell > 0) ? 1 : (delta_ftell < 0) ? -1 : 0 );Good find!
Please don't nest the ternary operator, it is hard to read.
Quoted 7 lines
if (delta_ftell < 0) {
return -1;
} else if (delta_ftell > 0) {
return 1;
} else {
return 0;
}Bah, that's for wimps ;)
return (delta_ftell > 0) - (delta_ftell < 0);
Look ma, no branches!