* [Qemu-devel] [PATCH] vmdk: byteswap VMDK4Header.desc_offset field
@ 2013-06-10 9:07 Stefan Hajnoczi
2013-06-10 14:04 ` Kevin Wolf
0 siblings, 1 reply; 6+ messages in thread
From: Stefan Hajnoczi @ 2013-06-10 9:07 UTC (permalink / raw)
To: qemu-devel; +Cc: Kevin Wolf, Fam Zheng, qemu-stable, Stefan Hajnoczi
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(-)
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
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [Qemu-devel] [PATCH] vmdk: byteswap VMDK4Header.desc_offset field
2013-06-10 9:07 [Qemu-devel] [PATCH] vmdk: byteswap VMDK4Header.desc_offset field Stefan Hajnoczi
@ 2013-06-10 14:04 ` Kevin Wolf
2013-06-10 14:32 ` Stefan Hajnoczi
0 siblings, 1 reply; 6+ messages in thread
From: Kevin Wolf @ 2013-06-10 14:04 UTC (permalink / raw)
To: Stefan Hajnoczi; +Cc: Fam Zheng, qemu-devel, qemu-stable
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
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [Qemu-devel] [PATCH] vmdk: byteswap VMDK4Header.desc_offset field
2013-06-10 14:04 ` Kevin Wolf
@ 2013-06-10 14:32 ` Stefan Hajnoczi
2013-06-10 14:36 ` Kevin Wolf
2013-06-13 2:06 ` Fam Zheng
0 siblings, 2 replies; 6+ messages in thread
From: Stefan Hajnoczi @ 2013-06-10 14:32 UTC (permalink / raw)
To: Kevin Wolf; +Cc: Fam Zheng, qemu-devel, Stefan Hajnoczi, qemu-stable
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
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [Qemu-devel] [PATCH] vmdk: byteswap VMDK4Header.desc_offset field
2013-06-10 14:32 ` Stefan Hajnoczi
@ 2013-06-10 14:36 ` Kevin Wolf
2013-06-13 2:06 ` Fam Zheng
1 sibling, 0 replies; 6+ messages in thread
From: Kevin Wolf @ 2013-06-10 14:36 UTC (permalink / raw)
To: Stefan Hajnoczi; +Cc: Fam Zheng, qemu-devel, Stefan Hajnoczi, qemu-stable
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
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [Qemu-devel] [PATCH] vmdk: byteswap VMDK4Header.desc_offset field
2013-06-10 14:32 ` Stefan Hajnoczi
2013-06-10 14:36 ` Kevin Wolf
@ 2013-06-13 2:06 ` Fam Zheng
2013-06-13 8:33 ` Stefan Hajnoczi
1 sibling, 1 reply; 6+ messages in thread
From: Fam Zheng @ 2013-06-13 2:06 UTC (permalink / raw)
To: Stefan Hajnoczi; +Cc: Kevin Wolf, qemu-devel, Stefan Hajnoczi, qemu-stable
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?
--
Fam
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [Qemu-devel] [PATCH] vmdk: byteswap VMDK4Header.desc_offset field
2013-06-13 2:06 ` Fam Zheng
@ 2013-06-13 8:33 ` Stefan Hajnoczi
0 siblings, 0 replies; 6+ messages in thread
From: Stefan Hajnoczi @ 2013-06-13 8:33 UTC (permalink / raw)
To: Kevin Wolf, Stefan Hajnoczi, qemu-devel, qemu-stable
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
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2013-06-13 8:33 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-06-10 9:07 [Qemu-devel] [PATCH] vmdk: byteswap VMDK4Header.desc_offset field Stefan Hajnoczi
2013-06-10 14:04 ` Kevin Wolf
2013-06-10 14:32 ` Stefan Hajnoczi
2013-06-10 14:36 ` Kevin Wolf
2013-06-13 2:06 ` Fam Zheng
2013-06-13 8:33 ` Stefan Hajnoczi
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).