[Qemu-devel] [PATCH 1/1] block migration: fix return value mismatch

[Qemu-devel] [PATCH 1/1] block migration: fix return value mismatch

[Gary Hook]

[-- Attachment #1: Type: text/plain, Size: 1286 bytes --]

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.  */


[-- Attachment #2: Type: text/html, Size: 3956 bytes --]

Re: [Qemu-devel] [PATCH 1/1] block migration: fix return value mismatch

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1948 bytes --]

On 11/12/2014 11:48 AM, Gary Hook wrote:
> 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.
> 

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').

> 
> 
> 
> diff -u -r a/block-migration.c b/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.


> --- 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 @@

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.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 539 bytes --]

Re: [Qemu-devel] [PATCH 1/1] block migration: fix return value mismatch

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> On 11/12/2014 11:48 AM, Gary Hook wrote:
>> 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.
>> 
>
> 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

Re: [Qemu-devel] [PATCH 1/1] block migration: fix return value mismatch

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 400 bytes --]

On Wed, Nov 12, 2014 at 06:48:18PM +0000, Gary Hook wrote:
> -    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;
}

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [PATCH 1/1] block migration: fix return value mismatch

[Markus Armbruster]

Stefan Hajnoczi <stefanha@gmail.com> writes:

> On Wed, Nov 12, 2014 at 06:48:18PM +0000, Gary Hook wrote:
>> -    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;
> }

Bah, that's for wimps ;)

    return (delta_ftell > 0) - (delta_ftell < 0);

Look ma, no branches!

Re: [Qemu-devel] [PATCH 1/1] block migration: fix return value mismatch

[Gary Hook]

[-- Attachment #1: Type: text/plain, Size: 908 bytes --]

On 11/13/14, 5:20 AM, "Stefan Hajnoczi" <stefanha@gmail.com<mailto:stefanha@gmail.com>> wrote:

On Wed, Nov 12, 2014 at 06:48:18PM +0000, Gary Hook wrote:
-    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;
}

Well, that format wasn¹t my first choice. I¹m still unclear on some of the
style requirements on this project, so I went with terse. Your rewrite
would have been my preference, but I¹m wondering if the syntax checker
will want the parentheses stripped as ³Unnecessary² despite how much they
aid in readability.

This one has caused us no end of pain. It¹s wonderful to eradicate it.


[-- Attachment #2: Type: text/html, Size: 2750 bytes --]

Re: [Qemu-devel] [PATCH 1/1] block migration: fix return value mismatch

[Gary Hook]

[-- Attachment #1: Type: text/plain, Size: 932 bytes --]

On 11/13/14, 6:46 AM, "Markus Armbruster" <armbru@redhat.com<mailto:armbru@redhat.com>> wrote:

Stefan Hajnoczi <stefanha@gmail.com<mailto:stefanha@gmail.com>> writes:

On Wed, Nov 12, 2014 at 06:48:18PM +0000, Gary Hook wrote:
-    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;
}

Bah, that's for wimps ;)

    return (delta_ftell > 0) - (delta_ftell < 0);

Look ma, no branches!

Ha-ha! Very good, but even less readable than the compressed ternary version,
IMO. This function only gets called once per migration; I don't see a few branches as performance-critical and worth the sacrifice of clarity as to intent.


[-- Attachment #2: Type: text/html, Size: 2789 bytes --]

Re: [Qemu-devel] [PATCH 1/1] block migration: fix return value mismatch

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1704 bytes --]

On 11/13/2014 09:44 AM, Gary Hook wrote:

[metacomment]

> On 11/13/14, 6:46 AM, "Markus Armbruster" <armbru@redhat.com<mailto:armbru@redhat.com>> wrote:
> 
> Stefan Hajnoczi <stefanha@gmail.com<mailto:stefanha@gmail.com>> writes:
> 
> On Wed, Nov 12, 2014 at 06:48:18PM +0000, Gary Hook wrote:

Your quoting style leaves a lot to be desired.  While you correctly
attributed multiple contributors of the material quoted below...

> -    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;
> }
> 
> Bah, that's for wimps ;)
> 
>     return (delta_ftell > 0) - (delta_ftell < 0);
> 
> Look ma, no branches!
> 
> Ha-ha! Very good, but even less readable than the compressed ternary version,
> IMO. This function only gets called once per migration; I don't see a few branches as performance-critical and worth the sacrifice of clarity as to intent.

...all three sets of contributions were listed with NO differentiation
in indentation or leading '>' or other indication which part was quoted
vs. which part was new material from you.  Most people on this list use
a REAL mail agent (such as mutt or Thunderbird), and avoid web mail,
precisely because most web mail implementations are absolutely lousy at
writing decently threaded conversations.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 539 bytes --]

Re: [Qemu-devel] [PATCH 1/1] block migration: fix return value mismatch

[Gary Hook]

On 11/13/14, 11:03 AM, "Eric Blake" <eblake@redhat.com> wrote:

>On 11/13/2014 09:44 AM, Gary Hook wrote:
>
>[metacomment]
>
>> On 11/13/14, 6:46 AM, "Markus Armbruster"
>><armbru@redhat.com<mailto:armbru@redhat.com>> wrote:
>> 
>> Stefan Hajnoczi <stefanha@gmail.com<mailto:stefanha@gmail.com>> writes:
>> 
>> On Wed, Nov 12, 2014 at 06:48:18PM +0000, Gary Hook wrote:
>
>Your quoting style leaves a lot to be desired.  While you correctly
>attributed multiple contributors of the material quoted below...
>
>> -    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;
>> }
>> 
>> Bah, that's for wimps ;)
>> 
>>     return (delta_ftell > 0) - (delta_ftell < 0);
>> 
>> Look ma, no branches!
>> 
>> Ha-ha! Very good, but even less readable than the compressed ternary
>>version,
>> IMO. This function only gets called once per migration; I don't see a
>>few branches as performance-critical and worth the sacrifice of clarity
>>as to intent.
>
>...all three sets of contributions were listed with NO differentiation
>in indentation or leading '>' or other indication which part was quoted
>vs. which part was new material from you.  Most people on this list use
>a REAL mail agent (such as mutt or Thunderbird), and avoid web mail,
>precisely because most web mail implementations are absolutely lousy at
>writing decently threaded conversations.
>
>-- 
>Eric Blake   eblake redhat com    +1-919-301-3266
>Libvirt virtualization library http://libvirt.org

Son of aŠ.  You¹re absolutely right. Outlook is not helping me out here at
all. Mea culpa. I never used to have this problem with xmh.

I¹ll pay more attention going forward.

Re: [Qemu-devel] [PATCH 1/1] block migration: fix return value mismatch

[Stefan Hajnoczi]

On Thu, Nov 13, 2014 at 5:32 PM, Gary Hook <gary.hook@nimboxx.com> wrote:
> On 11/13/14, 11:03 AM, "Eric Blake" <eblake@redhat.com> wrote:
>
>>On 11/13/2014 09:44 AM, Gary Hook wrote:
>>
>>[metacomment]
>>
>>> On 11/13/14, 6:46 AM, "Markus Armbruster"
>>><armbru@redhat.com<mailto:armbru@redhat.com>> wrote:
>>>
>>> Stefan Hajnoczi <stefanha@gmail.com<mailto:stefanha@gmail.com>> writes:
>>>
>>> On Wed, Nov 12, 2014 at 06:48:18PM +0000, Gary Hook wrote:
>>
>>Your quoting style leaves a lot to be desired.  While you correctly
>>attributed multiple contributors of the material quoted below...
>>
>>> -    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;
>>> }
>>>
>>> Bah, that's for wimps ;)
>>>
>>>     return (delta_ftell > 0) - (delta_ftell < 0);
>>>
>>> Look ma, no branches!
>>>
>>> Ha-ha! Very good, but even less readable than the compressed ternary
>>>version,
>>> IMO. This function only gets called once per migration; I don't see a
>>>few branches as performance-critical and worth the sacrifice of clarity
>>>as to intent.
>>
>>...all three sets of contributions were listed with NO differentiation
>>in indentation or leading '>' or other indication which part was quoted
>>vs. which part was new material from you.  Most people on this list use
>>a REAL mail agent (such as mutt or Thunderbird), and avoid web mail,
>>precisely because most web mail implementations are absolutely lousy at
>>writing decently threaded conversations.
>>
>>--
>>Eric Blake   eblake redhat com    +1-919-301-3266
>>Libvirt virtualization library http://libvirt.org
>
> Son of aŠ.  You¹re absolutely right. Outlook is not helping me out here at
> all. Mea culpa. I never used to have this problem with xmh.
>
> I¹ll pay more attention going forward.

One more thing about email configuration: the character set encoding
of your emails seems to be incorrect.

GMail is rendering a superscript 1 (like "to the power of one") when
you wanted a single quote:
https://en.wikipedia.org/wiki/Unicode_subscripts_and_superscripts

In your email message there is a 0xb9 value instead of an ASCII single
quote (for example, "I'll" is encoded "I=B9ll").

The headers claim the encoding is Windows-1252.  The problem is that
0xb9 is the superscript one character and not the single quote:
https://en.wikipedia.org/wiki/Windows-1252

I guess you didn't really send a Windows-1252 message but some other
8-bit encoding.

Stefan

Re: [Qemu-devel] [PATCH 1/1] block migration: fix return value mismatch

[Gary R Hook]

[-- Attachment #1: Type: text/plain, Size: 971 bytes --]



> Stefan Hajnoczi <mailto:stefanha@gmail.com>
> November 13, 2014 at 12:55 PM
>
> One more thing about email configuration: the character set encoding
> of your emails seems to be incorrect.
>
> GMail is rendering a superscript 1 (like "to the power of one") when
> you wanted a single quote:
> https://en.wikipedia.org/wiki/Unicode_subscripts_and_superscripts
>
> In your email message there is a 0xb9 value instead of an ASCII single
> quote (for example, "I'll" is encoded "I=B9ll").
>
> The headers claim the encoding is Windows-1252. The problem is that
> 0xb9 is the superscript one character and not the single quote:
> https://en.wikipedia.org/wiki/Windows-1252
>
> I guess you didn't really send a Windows-1252 message but some other
> 8-bit encoding.
>
> Stefan

Well, Outlook appears to have done some very strange things, things I 
should have turned off. Kicking the tires on Postbox with this one.... 
I'm not real sure about line wrap just yet, however.

[-- Attachment #2.1: Type: text/html, Size: 2656 bytes --]

[-- Attachment #2.2: postbox-contact.jpg --]
[-- Type: image/jpeg, Size: 1446 bytes --]

Re: [Qemu-devel] [PATCH 1/1] block migration: fix return value mismatch

[Gary R Hook]

Stefan Hajnoczi <address@hidden> writes:

 > On Wed, Nov 12, 2014 at 06:48:18PM +0000, Gary Hook wrote:
 >> -    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;
 > }

So now that it's clear that I fully botched the patch submission, I 
would like to follow up: shall I resubmit this change?

I don't see this fix in the current source (which makes sense due to the 
2.2 RC2 work) but folks are asking. And it appears that there's a bug 
report over at RedHat that may be the same problem.

I think I'll add to my .sig: "Look, ma, only 72 columns!"

-- 
Gary R Hook
Senior Kernel Engineer
NIMBOXX, Inc

Re: [Qemu-devel] [PATCH 1/1] block migration: fix return value mismatch

[Dr. David Alan Gilbert]

* Gary R Hook (grhookatwork@gmail.com) wrote:
> Stefan Hajnoczi <address@hidden> writes:
> 
> > On Wed, Nov 12, 2014 at 06:48:18PM +0000, Gary Hook wrote:
> >> -    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;
> > }
> 
> So now that it's clear that I fully botched the patch submission, I would
> like to follow up: shall I resubmit this change?
> 
> I don't see this fix in the current source (which makes sense due to the 2.2
> RC2 work) but folks are asking. And it appears that there's a bug report
> over at RedHat that may be the same problem.
> 
> I think I'll add to my .sig: "Look, ma, only 72 columns!"

Please resubmit it, especially with the correct Signed-off-by

Dave

> 
> -- 
> Gary R Hook
> Senior Kernel Engineer
> NIMBOXX, Inc
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK