Re: [Qemu-devel] [PATCH v2 2/4] softfloat: Avoid uint16 type conflict on Darwin
On Oct 31, 2011, at 3:18 PM, Andreas Färber wrote: In file included from ./bswap.h:7, from ./qemu-common.h:106, from ./qemu-aio.h:17, from ./Block.h:4, from /System/Library/Frameworks/ CoreServices.framework/Frameworks/CarbonCore.framework/Headers/ FSEvents.h:28, from /System/Library/Frameworks/ CoreServices.framework/Frameworks/CarbonCore.framework/Headers/ CarbonCore.h:218, from /System/Library/Frameworks/ CoreServices.framework/Frameworks/AE.framework/Headers/AE.h:20, from /System/Library/Frameworks/ CoreServices.framework/Headers/CoreServices.h:21, from /System/Library/Frameworks/Foundation.framework/ Headers/NSURLError.h:17, from /System/Library/Frameworks/Foundation.framework/ Headers/Foundation.h:81, from /System/Library/Frameworks/Cocoa.framework/ Headers/Cocoa.h:12, from ui/cocoa.m:25: /Users/andreas/QEMU/qemu/fpu/softfloat.h:60: error: conflicting types for ‘uint16’ /System/Library/Frameworks/Security.framework/Headers/cssmconfig.h: 73: error: previous declaration of ‘uint16’ was here make: *** [ui/cocoa.o] Error 1 Apple's FSEvents.h has #include Block.h, which wants /usr/include/Block.h but due to case-insensitive file system and include path jungle gets QEMU's ./block.h, which in turn includes softfloat.h indirectly. Therefore work around the conflict in softfloat.h itself by renaming specifically uint16 on Darwin to qemu_uint16. This fixes the build until we have a more general solution. Signed-off-by: Andreas Färber andreas.faer...@web.de Cc: Juan Pineda j...@logician.com Cc: Peter Maydell peter.mayd...@linaro.org --- fpu/softfloat.h |3 +++ 1 files changed, 3 insertions(+), 0 deletions(-) diff --git a/fpu/softfloat.h b/fpu/softfloat.h index 07c2929..5320945 100644 --- a/fpu/softfloat.h +++ b/fpu/softfloat.h @@ -54,6 +54,9 @@ these four paragraphs for those parts of this code that are retained. | to the same as `int'. **/ typedef uint8_t flag; +#ifdef __APPLE__ +#define uint16 qemu_uint16 +#endif typedef uint8_t uint8; typedef int8_t int8; #ifndef _AIX Perhaps the following alternative solution would be more palatable? It's still tremendously ugly, but is localized to cocoa.m, thus less intrusive. -- 8 -- Subject: [PATCH] softfloat: Avoid uint16 type conflict on Darwin cocoa.m includes Security/cssmconfig.h indirectly via Cocoa/Cocoa.h. cssmconfig.h defines type uint16 which unfortunately conflicts with the definition in qemu's softfloat.h, thus resulting in compilation failure. To work around the problem, #define _UINT16, which informs cssmconfig.h that uint16 is already defined and that it should not apply its own definition. Additionally, ensure that Cocoa/Cocoa.h is included after softfloat.h rather than before since some Cocoa headers expect type uint16 to exist. Signed-off-by: Eric Sunshine sunsh...@sunshineco.com --- ui/cocoa.m |7 --- 1 files changed, 4 insertions(+), 3 deletions(-) diff --git a/ui/cocoa.m b/ui/cocoa.m index d9e4e3d..ac15418 100644 --- a/ui/cocoa.m +++ b/ui/cocoa.m @@ -22,13 +22,14 @@ * THE SOFTWARE. */ -#import Cocoa/Cocoa.h -#include crt_externs.h - #include qemu-common.h #include console.h #include sysemu.h +#define _UINT16 +#import Cocoa/Cocoa.h +#include crt_externs.h + #ifndef MAC_OS_X_VERSION_10_4 #define MAC_OS_X_VERSION_10_4 1040 #endif -- 1.7.7.1
[Qemu-devel] [PATCH] qemu-barrier: Fix build failure on PowerPC Mac OS X
qemu-barrier.h tests if macro __powerpc__ is defined, however, the preprocessor on PowerPC Mac OS X defines only __POWERPC__, not __powerpc__. Resolve by testing instead for qemu-provided _ARCH_PPC. Signed-off-by: Eric Sunshine sunsh...@sunshineco.com --- The anomalous __powerpc__ test appears only in qemu-barrier.h. No other source files reference this name. Cc: David Gibson da...@gibson.dropbear.id.au qemu-barrier.h |2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/qemu-barrier.h b/qemu-barrier.h index 735eea6..c11bb2b 100644 --- a/qemu-barrier.h +++ b/qemu-barrier.h @@ -14,7 +14,7 @@ */ #define smp_wmb() barrier() -#elif defined(__powerpc__) +#elif defined(_ARCH_PPC) /* * We use an eieio() for a wmb() on powerpc. This assumes we don't -- 1.7.7.1
Re: [Qemu-devel] [PATCH v2 2/4] softfloat: Avoid uint16 type conflict on Darwin
On Nov 1, 2011, at 12:37 PM, Andreas Färber wrote: Am 01.11.2011 09:09, schrieb Eric Sunshine: Perhaps the following alternative solution would be more palatable? It's still tremendously ugly, but is localized to cocoa.m, thus less intrusive. -- 8 -- Subject: [PATCH] softfloat: Avoid uint16 type conflict on Darwin cocoa.m includes Security/cssmconfig.h indirectly via Cocoa/ Cocoa.h. cssmconfig.h defines type uint16 which unfortunately conflicts with the definition in qemu's softfloat.h, thus resulting in compilation failure. To work around the problem, #define _UINT16, which informs cssmconfig.h that uint16 is already defined and that it should not apply its own definition. Thanks for the suggestion! _UINT16 is an interesting suggestion, however softfloat's uint16 is not uint16_t but int, so I'd rather not do it that way around. (I had also decided against the AIX path of never defining uint16 and always using system definitions, since that wouldn't work outside Cocoa code.) Do you have any thoughts about the include path issue? If we could keep QEMU code from getting into #import Cocoa/Cocoa.h then we could redefine the system type instead, in cocoa.m. Is the intention to trust uint16 from Security/cssmconfig.h over the one softfloat.h? If so, shouldn't we be taking as many type definitions from Security/cssmconfig.h as we can rather than just this one? (I'm not recommending it; just trying to understand the goal.) -- ES
Re: [Qemu-devel] [PATCH v3 2/4] softfloat: Avoid uint16 type conflict on Darwin
On Nov 1, 2011, at 2:05 PM, Andreas Färber wrote: Am 01.11.2011 19:01, schrieb Peter Maydell: On 1 November 2011 17:59, Andreas Färber andreas.faer...@web.de wrote: Apple's FSEvents.h has #include Block.h, which wants /usr/include/Block.h but due to case-insensitive file system and include path jungle gets QEMU's ./block.h, which in turn includes softfloat.h indirectly. Incidentally, surely you need to fix this anyway (ie fix the include path issue somehow)? Yes, that's why I'm explicitly documenting it. I have no clue how to fix it though. Experts' opinion welcome! It probably is not a good idea to emphasize the particular #include stack issued by this one build environment (presumably XCode on Lion?) so loudly in the commit message. The type redefinition error will manifest regardless of #include ordering and regardless of whether or not ./block.h is picked up accidentally instead of Block.h. For instance, on Leopard with XCode 3.1.4, the #include stack emitted with the error message is entirely different even though the underlying issue is the same: OBJC ui/cocoa.o In file included from ./bswap.h:8, from ./qemu-common.h:107, from ui/cocoa.m:29: /Users/sunshine/Desktop/qemu/fpu/softfloat.h:60: error: conflicting types for 'uint16' /System/Library/Frameworks/Security.framework/Headers/cssmconfig.h:68: error: previous declaration of 'uint16' was here make: *** [ui/cocoa.o] Error 1 In fact, on Leopard, FSEvents.h does not #include Block.h, so focusing on it in the log message is misleading. -- ES
Re: [Qemu-devel] [PATCH v2 2/4] softfloat: Avoid uint16 type conflict on Darwin
On Nov 1, 2011, at 2:52 PM, Andreas Färber wrote: Am 01.11.2011 19:47, schrieb Eric Sunshine: On Nov 1, 2011, at 12:37 PM, Andreas Färber wrote: Am 01.11.2011 09:09, schrieb Eric Sunshine: Perhaps the following alternative solution would be more palatable? It's still tremendously ugly, but is localized to cocoa.m, thus less intrusive. -- 8 -- Subject: [PATCH] softfloat: Avoid uint16 type conflict on Darwin cocoa.m includes Security/cssmconfig.h indirectly via Cocoa/ Cocoa.h. cssmconfig.h defines type uint16 which unfortunately conflicts with the definition in qemu's softfloat.h, thus resulting in compilation failure. To work around the problem, #define _UINT16, which informs cssmconfig.h that uint16 is already defined and that it should not apply its own definition. Thanks for the suggestion! _UINT16 is an interesting suggestion, however softfloat's uint16 is not uint16_t but int, so I'd rather not do it that way around. (I had also decided against the AIX path of never defining uint16 and always using system definitions, since that wouldn't work outside Cocoa code.) Do you have any thoughts about the include path issue? If we could keep QEMU code from getting into #import Cocoa/Cocoa.h then we could redefine the system type instead, in cocoa.m. Is the intention to trust uint16 from Security/cssmconfig.h over the one softfloat.h? If so, shouldn't we be taking as many type definitions from Security/cssmconfig.h as we can rather than just this one? (I'm not recommending it; just trying to understand the goal.) Short-term goal: make Darwin build 1.0 without breaking others Long-term goal: not use uint16 etc. in QEMU at all Don't see what you mean with taking as many type definitions. After uint16 I get no further conflicts for --enable-system --disable-user, so what is there to take? Sorry for not being clear. My question was not about build errors but about semantics. What I meant was that, with this patch, you are giving special preference only to Darwin's definition of uint16, but then contrarily preferring softfloat's definition of int16. Likewise, softfloat's uint32, int32, uint64, int64 from softfloat are trusted over the definitions from Darwin. Other than the fact that only uint16 led to a compilation error, it does not make sense semantically to single out Darwin's definition of only this one type. I would think that we should be trusting either _all_ Darwin type definitions or _none_. Singling out just this one seems anomalous. -- ES
Re: [Qemu-devel] [PATCH v2 2/4] softfloat: Avoid uint16 type conflict on Darwin
On Nov 1, 2011, at 3:06 PM, Eric Sunshine wrote: On Nov 1, 2011, at 2:52 PM, Andreas Färber wrote: Am 01.11.2011 19:47, schrieb Eric Sunshine: On Nov 1, 2011, at 12:37 PM, Andreas Färber wrote: Am 01.11.2011 09:09, schrieb Eric Sunshine: Perhaps the following alternative solution would be more palatable? It's still tremendously ugly, but is localized to cocoa.m, thus less intrusive. -- 8 -- Subject: [PATCH] softfloat: Avoid uint16 type conflict on Darwin cocoa.m includes Security/cssmconfig.h indirectly via Cocoa/ Cocoa.h. cssmconfig.h defines type uint16 which unfortunately conflicts with the definition in qemu's softfloat.h, thus resulting in compilation failure. To work around the problem, #define _UINT16, which informs cssmconfig.h that uint16 is already defined and that it should not apply its own definition. Thanks for the suggestion! _UINT16 is an interesting suggestion, however softfloat's uint16 is not uint16_t but int, so I'd rather not do it that way around. (I had also decided against the AIX path of never defining uint16 and always using system definitions, since that wouldn't work outside Cocoa code.) Do you have any thoughts about the include path issue? If we could keep QEMU code from getting into #import Cocoa/Cocoa.h then we could redefine the system type instead, in cocoa.m. Is the intention to trust uint16 from Security/cssmconfig.h over the one softfloat.h? If so, shouldn't we be taking as many type definitions from Security/cssmconfig.h as we can rather than just this one? (I'm not recommending it; just trying to understand the goal.) Short-term goal: make Darwin build 1.0 without breaking others Long-term goal: not use uint16 etc. in QEMU at all Don't see what you mean with taking as many type definitions. After uint16 I get no further conflicts for --enable-system --disable-user, so what is there to take? Sorry for not being clear. My question was not about build errors but about semantics. What I meant was that, with this patch, you are giving special preference only to Darwin's definition of uint16, but then contrarily preferring softfloat's definition of int16. Likewise, softfloat's uint32, int32, uint64, int64 from softfloat are trusted over the definitions from Darwin. Other than the fact that only uint16 led to a compilation error, it does not make sense semantically to single out Darwin's definition of only this one type. I would think that we should be trusting either _all_ Darwin type definitions or _none_. Singling out just this one seems anomalous. -- ES I forgot to mention that with your patch, only cocoa.m is seeing Darwin's definition of uint16. The rest of qemu is seeing the definition from softfloat.h. This inconsistency hopefully is not harmful in the short-term, which is why I asked about the goal. If the short-term idea is for cocoa.m to build cleanly but not to worry much that cocoa.m sees a different uint16 from the rest of qemu, then the less intrusive patch involving only cocoa.m may be preferable. -- ES
Re: [Qemu-devel] [PATCH v2 2/4] softfloat: Avoid uint16 type conflict on Darwin
On Nov 1, 2011, at 3:25 PM, Andreas Färber wrote: Am 01.11.2011 20:06, schrieb Eric Sunshine: On Nov 1, 2011, at 2:52 PM, Andreas Färber wrote: Am 01.11.2011 19:47, schrieb Eric Sunshine: On Nov 1, 2011, at 12:37 PM, Andreas Färber wrote: Am 01.11.2011 09:09, schrieb Eric Sunshine: Perhaps the following alternative solution would be more palatable? It's still tremendously ugly, but is localized to cocoa.m, thus less intrusive. -- 8 -- Subject: [PATCH] softfloat: Avoid uint16 type conflict on Darwin cocoa.m includes Security/cssmconfig.h indirectly via Cocoa/Cocoa.h. cssmconfig.h defines type uint16 which unfortunately conflicts with the definition in qemu's softfloat.h, thus resulting in compilation failure. To work around the problem, #define _UINT16, which informs cssmconfig.h that uint16 is already defined and that it should not apply its own definition. Thanks for the suggestion! _UINT16 is an interesting suggestion, however softfloat's uint16 is not uint16_t but int, so I'd rather not do it that way around. (I had also decided against the AIX path of never defining uint16 and always using system definitions, since that wouldn't work outside Cocoa code.) Do you have any thoughts about the include path issue? If we could keep QEMU code from getting into #import Cocoa/Cocoa.h then we could redefine the system type instead, in cocoa.m. Is the intention to trust uint16 from Security/cssmconfig.h over the one softfloat.h? If so, shouldn't we be taking as many type definitions from Security/cssmconfig.h as we can rather than just this one? (I'm not recommending it; just trying to understand the goal.) Short-term goal: make Darwin build 1.0 without breaking others Long-term goal: not use uint16 etc. in QEMU at all Don't see what you mean with taking as many type definitions. After uint16 I get no further conflicts for --enable-system --disable- user, so what is there to take? Sorry for not being clear. My question was not about build errors but about semantics. What I meant was that, with this patch, you are giving special preference only to Darwin's definition of uint16, but then contrarily preferring softfloat's definition of int16. Likewise, softfloat's uint32, int32, uint64, int64 from softfloat are trusted over the definitions from Darwin. Other than the fact that only uint16 led to a compilation error, it does not make sense semantically to single out Darwin's definition of only this one type. I would think that we should be trusting either _all_ Darwin type definitions or _none_. Singling out just this one seems anomalous. Listen, I dont have time for this. We have three options: 1) I can say, I'm the Cocoa maintainer for multiple years now, I don't care if someone pops up day before the deadline and complains and just push my version of preference. I hope that you do not interpret my alternate patch as a complaint before the deadline. My intention only was to be helpful when I saw Peter's response [1], and thought that a less intrusive patch might be more acceptable. [1]: http://lists.nongnu.org/archive/html/qemu-devel/2011-10/msg03936.html -- ES
Re: [Qemu-devel] [PATCH] Teach block/vdi about discarded (no longer allocated) blocks
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 (0x) represents a never-allocated block (semantically arbitrary content). VDI_IMAGE_BLOCK_ZERO (0xfffe) 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 sunsh...@sunshineco.com 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 0xU #define VDI_DISCARD 0xfffeU 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 0xU, though. The 0xU 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) == 0xU conversion in his head. Consequently, the 0xU notation might a better choice, if it's not too much bother for you to amend the patch. -- ES
Re: [Qemu-devel] [PATCH 2/2] Documentation: Add syntax for using sheepdog devices
On Oct 28, 2011, at 5:13 AM, Ronnie Sahlberg wrote: Signed-off-by: Ronnie Sahlberg ronniesahlb...@gmail.com --- qemu-options.hx | 27 +++ 1 files changed, 27 insertions(+), 0 deletions(-) diff --git a/qemu-options.hx b/qemu-options.hx index c55080c..38d0f57 100644 --- a/qemu-options.hx +++ b/qemu-options.hx @@ -1778,6 +1778,33 @@ Example for Unix Domain Sockets qemu --drive file=nbd:unix:/tmp/nbd-socket @end example +@item Sheepdog +Sheepdog is a distributed storage system for QEMU. +QEMU sopports using either local sheepdog devices or remote networked s/sopports/supports/ -- ES
Re: [Qemu-devel] [PATCH] Documentation: Describe NBD URL syntax
On Oct 27, 2011, at 5:33 AM, Ronnie Sahlberg wrote: This patch adds a short description of how to specify a NBD device to QEMU. Syntax for both TCP and Unix Domain Sockets are provided as well as examples. Signed-off-by: Ronnie Sahlberg ronniesahlb...@gmail.com --- qemu-options.hx | 21 + 1 files changed, 21 insertions(+), 0 deletions(-) diff --git a/qemu-options.hx b/qemu-options.hx index 7c434f8..564ae3f 100644 --- a/qemu-options.hx +++ b/qemu-options.hx @@ -1757,6 +1757,27 @@ qemu --drive file=iscsi://192.0.2.1/iqn. 2001-04.com.example/1 iSCSI support is an optional feature of QEMU and only available when compiled and linked against libiscsi. +@item NBD +QEMU supports NBD (Network Block Devices) both using TCP protocol as well +as Unix Domain Sockets. + +Syntax for specifying a NDB device using TCP +``nbd:server-ip:port[:exportname=export]'' + +Syntax for specifying a NDB device using Unix Domain Sockets +``nbd:unix:domain-socket[:exportname=export]'' On the two Syntax for... lines: s/NDB/NBD/ -- ES
Re: [Qemu-devel] [PATCH] Teach block/vdi about discarded (no longer allocated) blocks
On Oct 27, 2011, at 12:12 PM, Stefan Weil wrote: 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 (0x) represents a never-allocated block (semantically arbitrary content). VDI_IMAGE_BLOCK_ZERO (0xfffe) 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 sunsh...@sunshineco.com 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 0xU #define VDI_DISCARD 0xfffeU Using the macro names and the definitions (with type cast) from the original VirtualBox code would also be ok. I originally implemented the change using the macro names, comments, and definitions from the VirtualBox code, but found that it made the diff so noisy that it obscured the simpler underlying change of teaching block/vdi about discarded blocks. Sticking with the original VDI_UNALLOCATED macro name kept the diff noise level down. At any rate, if Kevin can amend the commit with one of your above suggestions, that would be simplest. Otherwise, I can re-roll. Let me know which is preferred. -- ES
[Qemu-devel] [PATCH] Teach block/vdi about discarded (no longer allocated) blocks
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
(0x) represents a never-allocated block (semantically arbitrary
content). VDI_IMAGE_BLOCK_ZERO (0xfffe) 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 sunsh...@sunshineco.com
---
Without this patch, qemu-image check on a VDI image containing
discarded blocks reports errors such as:
ERROR: block index 3434 too large, is 4294967294
Decimal 4294967294 is 0xfffe. Worse, qemu-image convert or direct
access of the VDI image from qemu involves reads and writes of blocks at
the bogus block offset 4294967294 within the image file.
Cc: Stefan Weil w...@mail.berlios.de
Cc: Kevin Wolf kw...@redhat.com
block/vdi.c | 23 ++-
1 files changed, 14 insertions(+), 9 deletions(-)
diff --git a/block/vdi.c b/block/vdi.c
index 883046d..25790c4 100644
--- a/block/vdi.c
+++ b/block/vdi.c
@@ -114,8 +114,13 @@ void uuid_unparse(const uuid_t uu, char *out);
*/
#define VDI_TEXT QEMU VM Virtual Disk Image \n
-/* Unallocated blocks use this index (no need to convert endianness). */
-#define VDI_UNALLOCATED UINT32_MAX
+/* A never-allocated block; semantically arbitrary content. */
+#define VDI_UNALLOCATED ((uint32_t)~0)
+
+/* A discarded (no longer allocated) block; semantically zero-filled. */
+#define VDI_DISCARDED ((uint32_t)~1)
+
+#define VDI_IS_ALLOCATED(X) ((X) VDI_DISCARDED)
#if !defined(CONFIG_UUID)
void uuid_generate(uuid_t out)
@@ -307,10 +312,10 @@ static int vdi_check(BlockDriverState *bs,
BdrvCheckResult *res)
/* Check block map and value of blocks_allocated. */
for (block = 0; block s-header.blocks_in_image; block++) {
uint32_t bmap_entry = le32_to_cpu(s-bmap[block]);
-if (bmap_entry != VDI_UNALLOCATED) {
+if (VDI_IS_ALLOCATED(bmap_entry)) {
if (bmap_entry s-header.blocks_in_image) {
blocks_allocated++;
-if (bmap[bmap_entry] == VDI_UNALLOCATED) {
+if (!VDI_IS_ALLOCATED(bmap[bmap_entry])) {
bmap[bmap_entry] = bmap_entry;
} else {
fprintf(stderr, ERROR: block index % PRIu32
@@ -472,7 +477,7 @@ static int vdi_is_allocated(BlockDriverState *bs, int64_t
sector_num,
n_sectors = nb_sectors;
}
*pnum = n_sectors;
-return bmap_entry != VDI_UNALLOCATED;
+return VDI_IS_ALLOCATED(bmap_entry);
}
static void vdi_aio_cancel(BlockDriverAIOCB *blockacb)
@@ -603,7 +608,7 @@ static void vdi_aio_read_cb(void *opaque, int ret)
/* prepare next AIO request */
acb-n_sectors = n_sectors;
bmap_entry = le32_to_cpu(s-bmap[block_index]);
-if (bmap_entry == VDI_UNALLOCATED) {
+if (!VDI_IS_ALLOCATED(bmap_entry)) {
/* Block not allocated, return zeros, no need to wait. */
memset(acb-buf, 0, n_sectors * SECTOR_SIZE);
ret = vdi_schedule_bh(vdi_aio_rw_bh, acb);
@@ -685,7 +690,7 @@ static void vdi_aio_write_cb(void *opaque, int ret)
if (acb-header_modified) {
VdiHeader *header = acb-block_buffer;
logout(now writing modified header\n);
-assert(acb-bmap_first != VDI_UNALLOCATED);
+assert(VDI_IS_ALLOCATED(acb-bmap_first));
*header = s-header;
vdi_header_to_le(header);
acb-header_modified = 0;
@@ -699,7 +704,7 @@ static void vdi_aio_write_cb(void *opaque, int ret)
goto done;
}
return;
-} else if (acb-bmap_first != VDI_UNALLOCATED) {
+} else if (VDI_IS_ALLOCATED(acb-bmap_first)) {
/* One or more new blocks were allocated. */
uint64_t offset;
uint32_t bmap_first;
@@ -749,7 +754,7 @@ static void vdi_aio_write_cb(void *opaque, int ret)
/* prepare next AIO request */
acb-n_sectors = n_sectors;
bmap_entry = le32_to_cpu(s-bmap[block_index]);
-if (bmap_entry == VDI_UNALLOCATED) {
+if (!VDI_IS_ALLOCATED(bmap_entry)) {
/* Allocate new block and write to it. */
uint64_t offset;
uint8_t *block;
--
1.7.7.1
Re: [Qemu-devel] [PATCH] Teach block/vdi about discarded (no longer allocated) blocks
On Oct 26, 2011, at 4:24 PM, Stefan Weil wrote:
Thank you for this extension. I have several remarks - see below.
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
(0x) represents a never-allocated block (semantically
arbitrary
content). VDI_IMAGE_BLOCK_ZERO (0xfffe) 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 sunsh...@sunshineco.com
---
Without this patch, qemu-image check on a VDI image containing
discarded blocks reports errors such as:
ERROR: block index 3434 too large, is 4294967294
Decimal 4294967294 is 0xfffe. Worse, qemu-image convert or
direct
access of the VDI image from qemu involves reads and writes of
blocks at
the bogus block offset 4294967294 within the image file.
Cc: Stefan Weil w...@mail.berlios.de
Cc: Kevin Wolf kw...@redhat.com
block/vdi.c | 23 ++-
1 files changed, 14 insertions(+), 9 deletions(-)
diff --git a/block/vdi.c b/block/vdi.c
index 883046d..25790c4 100644
--- a/block/vdi.c
+++ b/block/vdi.c
@@ -114,8 +114,13 @@ void uuid_unparse(const uuid_t uu, char *out);
*/
#define VDI_TEXT QEMU VM Virtual Disk Image \n
-/* Unallocated blocks use this index (no need to convert
endianness). */
-#define VDI_UNALLOCATED UINT32_MAX
+/* A never-allocated block; semantically arbitrary content. */
+#define VDI_UNALLOCATED ((uint32_t)~0)
Why did you change the definition of VDI_UNALLOCATED?
Or do you get a difference with the old definition?
My hope was that future readers of the code might find it easier to
assimilate if it used the same notation (uint32_t)~0 as the
VirtualBox source code (which also is the most accurate documentation
of the VDI format). I don't have particularly strong feelings about it
and can re-roll using UINT32_MAX if you prefer.
It's ok to change the comment, but you missed an important point
(endianness).
The removal of the comment was intentional because it was ambiguous
and confusing rather than illuminating. Specifically, it does not
explain if this is a case of programmer laziness (0x being the
same on big- and little-endian) or if code employing VDI_UNALLOCATED
applies proper endian conversions. Had the comment indicated that
VDI_UNALLOCATED is only ever employed with host-endian values (which
is the case), then that would have been worth retaining. I can re-roll
with a clearer comment but would be sorry to see the confusing comment
retained.
+
+/* A discarded (no longer allocated) block; semantically zero-
filled. */
+#define VDI_DISCARDED ((uint32_t)~1)
The type cast is not needed. Please use
#define VDI_DISCARD (VDI_UNALLOCATED - 1)
+
+#define VDI_IS_ALLOCATED(X) ((X) VDI_DISCARDED)
#if !defined(CONFIG_UUID)
void uuid_generate(uuid_t out)
@@ -307,10 +312,10 @@ static int vdi_check(BlockDriverState *bs,
BdrvCheckResult *res)
/* Check block map and value of blocks_allocated. */
for (block = 0; block s-header.blocks_in_image; block++) {
uint32_t bmap_entry = le32_to_cpu(s-bmap[block]);
- if (bmap_entry != VDI_UNALLOCATED) {
+ if (VDI_IS_ALLOCATED(bmap_entry)) {
if (bmap_entry s-header.blocks_in_image) {
blocks_allocated++;
- if (bmap[bmap_entry] == VDI_UNALLOCATED) {
+ if (!VDI_IS_ALLOCATED(bmap[bmap_entry])) {
bmap[bmap_entry] = bmap_entry;
} else {
fprintf(stderr, ERROR: block index % PRIu32
@@ -472,7 +477,7 @@ static int vdi_is_allocated(BlockDriverState
*bs, int64_t sector_num,
n_sectors = nb_sectors;
}
*pnum = n_sectors;
- return bmap_entry != VDI_UNALLOCATED;
+ return VDI_IS_ALLOCATED(bmap_entry);
}
static void vdi_aio_cancel(BlockDriverAIOCB *blockacb)
@@ -603,7 +608,7 @@ static void vdi_aio_read_cb(void *opaque, int
ret)
/* prepare next AIO request */
acb-n_sectors = n_sectors;
bmap_entry = le32_to_cpu(s-bmap[block_index]);
- if (bmap_entry == VDI_UNALLOCATED) {
+ if (!VDI_IS_ALLOCATED(bmap_entry)) {
/* Block not allocated, return zeros, no need to wait. */
memset(acb-buf, 0, n_sectors * SECTOR_SIZE);
ret = vdi_schedule_bh(vdi_aio_rw_bh, acb);
@@ -685,7 +690,7 @@ static void vdi_aio_write_cb(void *opaque, int
ret)
if (acb-header_modified) {
VdiHeader *header = acb-block_buffer;
logout(now writing modified header\n);
- assert(acb-bmap_first != VDI_UNALLOCATED);
+ assert(VDI_IS_ALLOCATED(acb-bmap_first));
*header = s-header;
vdi_header_to_le(header);
acb-header_modified = 0;
@@ -699,7 +704,7 @@ static void vdi_aio_write_cb(void *opaque, int
ret)
goto done;
}
return;
- } else if (acb-bmap_first != VDI_UNALLOCATED) {
+ } else if (VDI_IS_ALLOCATED(acb-bmap_first)) {
/* One or more new blocks were allocated. */
uint64_t offset;
uint32_t bmap_first;
@@ -749,7 +754,7 @@ static void vdi_aio_write_cb(void
