Re: [Qemu-devel] [PATCH] vmdk: byteswap VMDK4Header.desc_offset field
On Thu, Jun 13, 2013 at 10:06:43AM +0800, Fam Zheng wrote:
On Mon, 06/10 16:32, Stefan Hajnoczi wrote:
On Mon, Jun 10, 2013 at 04:04:55PM +0200, Kevin Wolf wrote:
Am 10.06.2013 um 11:07 hat Stefan Hajnoczi geschrieben:
Remember to byteswap VMDK4Header.desc_offset on big-endian machines.
Cc: qemu-sta...@nongnu.org
Signed-off-by: Stefan Hajnoczi stefa...@redhat.com
Thanks, applied to the block layer.
@@ -507,8 +507,11 @@ static int vmdk_open_vmdk4(BlockDriverState *bs,
if (ret 0) {
return ret;
}
-if (header.capacity == 0 header.desc_offset) {
-return vmdk_open_desc_file(bs, flags, header.desc_offset 9);
+if (header.capacity == 0) {
+int64_t desc_offset = le64_to_cpu(header.desc_offset);
+if (desc_offset) {
+return vmdk_open_desc_file(bs, flags, desc_offset 9);
+}
}
Splitting up the if condition wouldn't have been necessary, strictly
speaking. But I don't mind too much here.
True. The reason I did it is because accessing header.desc_offset
directly is a bad habit. Someone modifying the code might conclude it's
safe to access directly when it actually only works for the limited
cases of zero and non-zero.
Not byteswapping header.capacity here, any reason?
Byteswapping header.capacity too is fine by me. This patch is focussed
on just header.desc_offset.
Stefan
Re: [Qemu-devel] [PATCH] vmdk: byteswap VMDK4Header.desc_offset field
On Mon, 06/10 16:32, Stefan Hajnoczi wrote:
On Mon, Jun 10, 2013 at 04:04:55PM +0200, Kevin Wolf wrote:
Am 10.06.2013 um 11:07 hat Stefan Hajnoczi geschrieben:
Remember to byteswap VMDK4Header.desc_offset on big-endian machines.
Cc: qemu-sta...@nongnu.org
Signed-off-by: Stefan Hajnoczi stefa...@redhat.com
Thanks, applied to the block layer.
@@ -507,8 +507,11 @@ static int vmdk_open_vmdk4(BlockDriverState *bs,
if (ret 0) {
return ret;
}
-if (header.capacity == 0 header.desc_offset) {
-return vmdk_open_desc_file(bs, flags, header.desc_offset 9);
+if (header.capacity == 0) {
+int64_t desc_offset = le64_to_cpu(header.desc_offset);
+if (desc_offset) {
+return vmdk_open_desc_file(bs, flags, desc_offset 9);
+}
}
Splitting up the if condition wouldn't have been necessary, strictly
speaking. But I don't mind too much here.
True. The reason I did it is because accessing header.desc_offset
directly is a bad habit. Someone modifying the code might conclude it's
safe to access directly when it actually only works for the limited
cases of zero and non-zero.
Not byteswapping header.capacity here, any reason?
--
Fam
[Qemu-devel] [PATCH] vmdk: byteswap VMDK4Header.desc_offset field
Remember to byteswap VMDK4Header.desc_offset on big-endian machines.
Cc: qemu-sta...@nongnu.org
Signed-off-by: Stefan Hajnoczi stefa...@redhat.com
---
block/vmdk.c | 7 +--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/block/vmdk.c b/block/vmdk.c
index 608daaf..ee50a73 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -507,8 +507,11 @@ static int vmdk_open_vmdk4(BlockDriverState *bs,
if (ret 0) {
return ret;
}
-if (header.capacity == 0 header.desc_offset) {
-return vmdk_open_desc_file(bs, flags, header.desc_offset 9);
+if (header.capacity == 0) {
+int64_t desc_offset = le64_to_cpu(header.desc_offset);
+if (desc_offset) {
+return vmdk_open_desc_file(bs, flags, desc_offset 9);
+}
}
if (le64_to_cpu(header.gd_offset) == VMDK4_GD_AT_END) {
--
1.8.1.4
Re: [Qemu-devel] [PATCH] vmdk: byteswap VMDK4Header.desc_offset field
Am 10.06.2013 um 11:07 hat Stefan Hajnoczi geschrieben:
Remember to byteswap VMDK4Header.desc_offset on big-endian machines.
Cc: qemu-sta...@nongnu.org
Signed-off-by: Stefan Hajnoczi stefa...@redhat.com
Thanks, applied to the block layer.
@@ -507,8 +507,11 @@ static int vmdk_open_vmdk4(BlockDriverState *bs,
if (ret 0) {
return ret;
}
-if (header.capacity == 0 header.desc_offset) {
-return vmdk_open_desc_file(bs, flags, header.desc_offset 9);
+if (header.capacity == 0) {
+int64_t desc_offset = le64_to_cpu(header.desc_offset);
+if (desc_offset) {
+return vmdk_open_desc_file(bs, flags, desc_offset 9);
+}
}
Splitting up the if condition wouldn't have been necessary, strictly
speaking. But I don't mind too much here.
Kevin
Re: [Qemu-devel] [PATCH] vmdk: byteswap VMDK4Header.desc_offset field
Am 10.06.2013 um 16:32 hat Stefan Hajnoczi geschrieben:
On Mon, Jun 10, 2013 at 04:04:55PM +0200, Kevin Wolf wrote:
Am 10.06.2013 um 11:07 hat Stefan Hajnoczi geschrieben:
Remember to byteswap VMDK4Header.desc_offset on big-endian machines.
Cc: qemu-sta...@nongnu.org
Signed-off-by: Stefan Hajnoczi stefa...@redhat.com
Thanks, applied to the block layer.
@@ -507,8 +507,11 @@ static int vmdk_open_vmdk4(BlockDriverState *bs,
if (ret 0) {
return ret;
}
-if (header.capacity == 0 header.desc_offset) {
-return vmdk_open_desc_file(bs, flags, header.desc_offset 9);
+if (header.capacity == 0) {
+int64_t desc_offset = le64_to_cpu(header.desc_offset);
+if (desc_offset) {
+return vmdk_open_desc_file(bs, flags, desc_offset 9);
+}
}
Splitting up the if condition wouldn't have been necessary, strictly
speaking. But I don't mind too much here.
True. The reason I did it is because accessing header.desc_offset
directly is a bad habit. Someone modifying the code might conclude it's
safe to access directly when it actually only works for the limited
cases of zero and non-zero.
It just looks a bit weird because you're still doing the same for
header.capacity and there's no real reason for treating the two fields
differently.
Kevin
Re: [Qemu-devel] [PATCH] vmdk: byteswap VMDK4Header.desc_offset field
On Mon, Jun 10, 2013 at 04:04:55PM +0200, Kevin Wolf wrote:
Am 10.06.2013 um 11:07 hat Stefan Hajnoczi geschrieben:
Remember to byteswap VMDK4Header.desc_offset on big-endian machines.
Cc: qemu-sta...@nongnu.org
Signed-off-by: Stefan Hajnoczi stefa...@redhat.com
Thanks, applied to the block layer.
@@ -507,8 +507,11 @@ static int vmdk_open_vmdk4(BlockDriverState *bs,
if (ret 0) {
return ret;
}
-if (header.capacity == 0 header.desc_offset) {
-return vmdk_open_desc_file(bs, flags, header.desc_offset 9);
+if (header.capacity == 0) {
+int64_t desc_offset = le64_to_cpu(header.desc_offset);
+if (desc_offset) {
+return vmdk_open_desc_file(bs, flags, desc_offset 9);
+}
}
Splitting up the if condition wouldn't have been necessary, strictly
speaking. But I don't mind too much here.
True. The reason I did it is because accessing header.desc_offset
directly is a bad habit. Someone modifying the code might conclude it's
safe to access directly when it actually only works for the limited
cases of zero and non-zero.
Stefan
