[Qemu-devel] [PATCH] block.h: Make BDRV_SECTOR_SIZE 64 bit safe

[Qemu-devel] [PATCH] block.h: Make BDRV_SECTOR_SIZE 64 bit safe

[Jes.Sorensen]

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(-)

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 {
-- 
1.6.5.2

Re: [Qemu-devel] [PATCH] block.h: Make BDRV_SECTOR_SIZE 64 bit safe

[Kevin Wolf]

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

[Qemu-devel] Re: [PATCH] block.h: Make BDRV_SECTOR_SIZE 64 bit safe

[Paolo Bonzini]

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

[Qemu-devel] Re: [PATCH] block.h: Make BDRV_SECTOR_SIZE 64 bit safe

[Jes Sorensen]

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

[Qemu-devel] Re: [PATCH] block.h: Make BDRV_SECTOR_SIZE 64 bit safe

[Paolo Bonzini]

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

[Qemu-devel] Re: [PATCH] block.h: Make BDRV_SECTOR_SIZE 64 bit safe

[Jes Sorensen]

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