qemu-trivial
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-trivial] [Qemu-devel] [PATCH] Teach block/vdi about "discarded


From: Eric Sunshine
Subject: Re: [Qemu-trivial] [Qemu-devel] [PATCH] Teach block/vdi about "discarded" (no longer allocated) blocks
Date: Fri, 28 Oct 2011 04:15:28 -0400


On Oct 28, 2011, at 4:00 AM, Kevin Wolf wrote:

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.

The 0xffffffffU notation has the benefit of being explicit, whereas the ((uint32_t)~0) notation, taken from the VirtualBox source, is somewhat magical for a reader who does not perform an automatic ((uint32_t)~0) == 0xffffffffU conversion in his head. Consequently, the 0xffffffffU notation might a better choice, if it's not too much bother for you to amend the patch.

-- ES




reply via email to

[Prev in Thread] Current Thread [Next in Thread]