| Message ID | 1370855253-7842-1-git-send-email-stefanha@redhat.com |
|---|---|
| State | New |
| Headers | show |
Am 10.06.2013 um 11:07 hat Stefan Hajnoczi geschrieben: > Remember to byteswap VMDK4Header.desc_offset on big-endian machines. > > Cc: qemu-stable@nongnu.org > Signed-off-by: Stefan Hajnoczi <stefanha@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
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-stable@nongnu.org > > Signed-off-by: Stefan Hajnoczi <stefanha@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
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-stable@nongnu.org > > > Signed-off-by: Stefan Hajnoczi <stefanha@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
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-stable@nongnu.org > > > Signed-off-by: Stefan Hajnoczi <stefanha@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?
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-stable@nongnu.org > > > > Signed-off-by: Stefan Hajnoczi <stefanha@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
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) {
Remember to byteswap VMDK4Header.desc_offset on big-endian machines. Cc: qemu-stable@nongnu.org Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> --- block/vmdk.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)