block.h: Make BDRV_SECTOR_SIZE 64 bit safe

Submitted by Jes Sorensen on May 27, 2010, 1:46 p.m.

Details

Message ID 1274968015-23599-1-git-send-email-Jes.Sorensen@redhat.com
State New
Headers show

Commit Message

Jes Sorensen May 27, 2010, 1:46 p.m.
From: Jes Sorensen <Jes.Sorensen@redhat.com>

C defaults to int, so make definition of BDRV_SECTOR_SIZE 64 bit
safe as it and BDRV_SECTOR_MASK may be used against 64 bit addresses.

Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>
---
 block.h |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

Comments

Kevin Wolf May 27, 2010, 2:27 p.m.
Am 27.05.2010 15:46, schrieb Jes.Sorensen@redhat.com:
> From: Jes Sorensen <Jes.Sorensen@redhat.com>
> 
> C defaults to int, so make definition of BDRV_SECTOR_SIZE 64 bit
> safe as it and BDRV_SECTOR_MASK may be used against 64 bit addresses.
> 
> Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>

Thanks, applied to the block branch.

Kevin
Paolo Bonzini May 27, 2010, 3:38 p.m.
On 05/27/2010 04:27 PM, Kevin Wolf wrote:
> Am 27.05.2010 15:46, schrieb Jes.Sorensen@redhat.com:
>> From: Jes Sorensen<Jes.Sorensen@redhat.com>
>>
>> C defaults to int, so make definition of BDRV_SECTOR_SIZE 64 bit
>> safe as it and BDRV_SECTOR_MASK may be used against 64 bit addresses.
>>
>> Signed-off-by: Jes Sorensen<Jes.Sorensen@redhat.com>
>
> Thanks, applied to the block branch.

Candidate for stable too?

Paolo
Jes Sorensen May 27, 2010, 3:44 p.m.
On 05/27/10 17:38, Paolo Bonzini wrote:
> On 05/27/2010 04:27 PM, Kevin Wolf wrote:
>> Am 27.05.2010 15:46, schrieb Jes.Sorensen@redhat.com:
>>> From: Jes Sorensen<Jes.Sorensen@redhat.com>
>>>
>>> C defaults to int, so make definition of BDRV_SECTOR_SIZE 64 bit
>>> safe as it and BDRV_SECTOR_MASK may be used against 64 bit addresses.
>>>
>>> Signed-off-by: Jes Sorensen<Jes.Sorensen@redhat.com>
>>
>> Thanks, applied to the block branch.
> 
> Candidate for stable too?

It should be safe to apply, but I didn't find any current users where
the mask was applied in a way where it was causing problems. Not sure if
you want the noise, or apply it as better safe than sorry?

Cheers,
Jes
Paolo Bonzini May 28, 2010, 8:32 a.m.
On 05/27/2010 05:44 PM, Jes Sorensen wrote:
> >  Candidate for stable too?
> It should be safe to apply, but I didn't find any current users where
> the mask was applied in a way where it was causing problems. Not sure if
> you want the noise, or apply it as better safe than sorry?

The only use in fact is this:

         addr = qemu_get_be64(f);
         flags = addr & ~BDRV_SECTOR_MASK;

which is safe since the ~~ cancels to give back 511 again.  So 
nevermind, just asking.  If there are no bugs related to it it seems 
just as safe not to apply it.

Paolo
Jes Sorensen May 28, 2010, 9:38 a.m.
On 05/28/10 10:32, Paolo Bonzini wrote:
> On 05/27/2010 05:44 PM, Jes Sorensen wrote:
>> >  Candidate for stable too?
>> It should be safe to apply, but I didn't find any current users where
>> the mask was applied in a way where it was causing problems. Not sure if
>> you want the noise, or apply it as better safe than sorry?
> 
> The only use in fact is this:
> 
>         addr = qemu_get_be64(f);
>         flags = addr & ~BDRV_SECTOR_MASK;
> 
> which is safe since the ~~ cancels to give back 511 again.  So
> nevermind, just asking.  If there are no bugs related to it it seems
> just as safe not to apply it.

That is correct, which is why I don't think it is necessary for the
stable release. However I want to see the fix in upstream as the macro
is likely to get used for other things in the future and it's a hidden
bug waiting to happen.

Cheers,
Jes

Patch hide | download patch | download mbox

diff --git a/block.h b/block.h
index 24efeb6..5e05612 100644
--- a/block.h
+++ b/block.h
@@ -38,7 +38,7 @@  typedef struct QEMUSnapshotInfo {
 #define BDRV_O_CACHE_MASK  (BDRV_O_NOCACHE | BDRV_O_CACHE_WB)
 
 #define BDRV_SECTOR_BITS   9
-#define BDRV_SECTOR_SIZE   (1 << BDRV_SECTOR_BITS)
+#define BDRV_SECTOR_SIZE   (1ULL << BDRV_SECTOR_BITS)
 #define BDRV_SECTOR_MASK   ~(BDRV_SECTOR_SIZE - 1)
 
 typedef enum {