Re: [Qemu-trivial] [Qemu-devel] [PATCH] Teach block/vdi about "discarded" (no longer allocated) blocks

[Kevin Wolf]

Am 27.10.2011 18:12, schrieb Stefan Weil:
> Am 27.10.2011 10:53, schrieb Kevin Wolf:
>> Am 26.10.2011 21:51, schrieb Eric Sunshine:
>>> An entry in the VDI block map will hold an offset to the actual block if
>>> the block is allocated, or one of two specially-interpreted values if
>>> not allocated. Using VirtualBox terminology, value VDI_IMAGE_BLOCK_FREE
>>> (0xffffffff) represents a never-allocated block (semantically arbitrary
>>> content). VDI_IMAGE_BLOCK_ZERO (0xfffffffe) represents a "discarded"
>>> block (semantically zero-filled). block/vdi knows only about
>>> VDI_IMAGE_BLOCK_FREE. Teach it about VDI_IMAGE_BLOCK_ZERO.
>>>
>>> Signed-off-by: Eric Sunshine <address@hidden>
>>
>> Thanks, applied to the block branch.
>>
>> Kevin
> 
> 
> Kevin, I don't want to block improvements. Nevertheless
> I'd like to see a small modification in this patch:
> both #defines should be implemented without a type cast.
> Please change them or wait until Eric sends an update.
> 
> My favorite is this:
> 
> #define VDI_UNALLOCATED UINT32_MAX
> #define VDI_DISCARD     (VDI_UNALLOCATED - 1)
> 
> This would also be ok:
> 
> #define VDI_UNALLOCATED 0xffffffffU
> #define VDI_DISCARD     0xfffffffeU
> 
> Using the macro names and the definitions (with type cast)
> from the original VirtualBox code would also be ok.

I did see your comments, and I waited for the endianness thing to be
answered. However, how the definition of these constants is written is
really not a functional defect, but simply a matter of taste. It's an
old rule that whoever does the work also decides on the details.

I really think it's wasting our time if we need to discuss if a type
cast in the constant definition is only allowed after typedefing
uint32_t to something else like in VBox.

So my preferred way is to leave the patch as it is. The code is simple
and clear and objectively seen it won't get any better with your taste
applied. If Eric prefers, I can update it to use 0xffffffffU, though.

Kevin