| Message ID | 1384303994-26796-2-git-send-email-famz@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Wed, Nov 13, 2013 at 08:53:10AM +0800, Fam Zheng wrote: > If target block driver forces compression, qemu-img convert needs to > write by cluster size as well as "-c" option. > > Particularly, this applies for converting to VMDK streamOptimized > format. > > Signed-off-by: Fam Zheng <famz@redhat.com> > --- > include/block/block.h | 1 + > qemu-img.c | 5 ++++- > 2 files changed, 5 insertions(+), 1 deletion(-) > > diff --git a/include/block/block.h b/include/block/block.h > index 3560deb..169c092 100644 > --- a/include/block/block.h > +++ b/include/block/block.h > @@ -18,6 +18,7 @@ typedef struct BlockDriverInfo { > /* offset at which the VM state can be saved (0 if not possible) */ > int64_t vm_state_offset; > bool is_dirty; > + bool is_compressed; > } BlockDriverInfo; > > typedef struct BlockFragInfo { > diff --git a/qemu-img.c b/qemu-img.c > index bf3fb4f..09ed9b2 100644 > --- a/qemu-img.c > +++ b/qemu-img.c > @@ -1383,8 +1383,11 @@ static int img_convert(int argc, char **argv) > } > } > > + ret = bdrv_get_info(out_bs, &bdi); > + if (ret == 0) { > + compress = compress || bdi.is_compressed; > + } > if (compress) { > - ret = bdrv_get_info(out_bs, &bdi); > if (ret < 0) { > error_report("could not get block driver info"); > goto out; You need to move the error checking up as well, above the 'if (compress)' Jeff
On 2013年11月13日 23:23, Jeff Cody wrote: > On Wed, Nov 13, 2013 at 08:53:10AM +0800, Fam Zheng wrote: >> If target block driver forces compression, qemu-img convert needs to >> write by cluster size as well as "-c" option. >> >> Particularly, this applies for converting to VMDK streamOptimized >> format. >> >> Signed-off-by: Fam Zheng <famz@redhat.com> >> --- >> include/block/block.h | 1 + >> qemu-img.c | 5 ++++- >> 2 files changed, 5 insertions(+), 1 deletion(-) >> >> diff --git a/include/block/block.h b/include/block/block.h >> index 3560deb..169c092 100644 >> --- a/include/block/block.h >> +++ b/include/block/block.h >> @@ -18,6 +18,7 @@ typedef struct BlockDriverInfo { >> /* offset at which the VM state can be saved (0 if not possible) */ >> int64_t vm_state_offset; >> bool is_dirty; >> + bool is_compressed; >> } BlockDriverInfo; >> >> typedef struct BlockFragInfo { >> diff --git a/qemu-img.c b/qemu-img.c >> index bf3fb4f..09ed9b2 100644 >> --- a/qemu-img.c >> +++ b/qemu-img.c >> @@ -1383,8 +1383,11 @@ static int img_convert(int argc, char **argv) >> } >> } >> >> + ret = bdrv_get_info(out_bs, &bdi); >> + if (ret == 0) { >> + compress = compress || bdi.is_compressed; >> + } >> if (compress) { >> - ret = bdrv_get_info(out_bs, &bdi); >> if (ret < 0) { >> error_report("could not get block driver info"); >> goto out; > > You need to move the error checking up as well, above the > 'if (compress)' > Moving it up forces bdrv_get_info to succeed for both compress case and non compress case, which is too strict and fails when target driver doesn't support it. I see it's just odd to assign ret outside if block and check it in only one branch, but I don't have a better way to do this here. Fam
On Thu, Nov 14, 2013 at 09:18:35AM +0800, Fam Zheng wrote: > On 2013年11月13日 23:23, Jeff Cody wrote: > >On Wed, Nov 13, 2013 at 08:53:10AM +0800, Fam Zheng wrote: > >>If target block driver forces compression, qemu-img convert needs to > >>write by cluster size as well as "-c" option. > >> > >>Particularly, this applies for converting to VMDK streamOptimized > >>format. > >> > >>Signed-off-by: Fam Zheng <famz@redhat.com> > >>--- > >> include/block/block.h | 1 + > >> qemu-img.c | 5 ++++- > >> 2 files changed, 5 insertions(+), 1 deletion(-) > >> > >>diff --git a/include/block/block.h b/include/block/block.h > >>index 3560deb..169c092 100644 > >>--- a/include/block/block.h > >>+++ b/include/block/block.h > >>@@ -18,6 +18,7 @@ typedef struct BlockDriverInfo { > >> /* offset at which the VM state can be saved (0 if not possible) */ > >> int64_t vm_state_offset; > >> bool is_dirty; > >>+ bool is_compressed; > >> } BlockDriverInfo; > >> > >> typedef struct BlockFragInfo { > >>diff --git a/qemu-img.c b/qemu-img.c > >>index bf3fb4f..09ed9b2 100644 > >>--- a/qemu-img.c > >>+++ b/qemu-img.c > >>@@ -1383,8 +1383,11 @@ static int img_convert(int argc, char **argv) > >> } > >> } > >> > >>+ ret = bdrv_get_info(out_bs, &bdi); > >>+ if (ret == 0) { > >>+ compress = compress || bdi.is_compressed; > >>+ } > >> if (compress) { > >>- ret = bdrv_get_info(out_bs, &bdi); > >> if (ret < 0) { > >> error_report("could not get block driver info"); > >> goto out; > > > >You need to move the error checking up as well, above the > >'if (compress)' > > > > Moving it up forces bdrv_get_info to succeed for both compress case > and non compress case, which is too strict and fails when target > driver doesn't support it. bdrv_get_info() will return -ENOTSUP if it is not supported. If we do call down to the driver layer bdrv_get_info() and get an error, I think we should pass it up. So just ignore -ENOTSUP if we don't care about the case when bdrv_get_info() is not supported by a target. > > I see it's just odd to assign ret outside if block and check it in > only one branch, but I don't have a better way to do this here. > > Fam >
On 2013年11月14日 09:34, Jeff Cody wrote: > On Thu, Nov 14, 2013 at 09:18:35AM +0800, Fam Zheng wrote: >> On 2013年11月13日 23:23, Jeff Cody wrote: >>> On Wed, Nov 13, 2013 at 08:53:10AM +0800, Fam Zheng wrote: >>>> If target block driver forces compression, qemu-img convert needs to >>>> write by cluster size as well as "-c" option. >>>> >>>> Particularly, this applies for converting to VMDK streamOptimized >>>> format. >>>> >>>> Signed-off-by: Fam Zheng <famz@redhat.com> >>>> --- >>>> include/block/block.h | 1 + >>>> qemu-img.c | 5 ++++- >>>> 2 files changed, 5 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/include/block/block.h b/include/block/block.h >>>> index 3560deb..169c092 100644 >>>> --- a/include/block/block.h >>>> +++ b/include/block/block.h >>>> @@ -18,6 +18,7 @@ typedef struct BlockDriverInfo { >>>> /* offset at which the VM state can be saved (0 if not possible) */ >>>> int64_t vm_state_offset; >>>> bool is_dirty; >>>> + bool is_compressed; >>>> } BlockDriverInfo; >>>> >>>> typedef struct BlockFragInfo { >>>> diff --git a/qemu-img.c b/qemu-img.c >>>> index bf3fb4f..09ed9b2 100644 >>>> --- a/qemu-img.c >>>> +++ b/qemu-img.c >>>> @@ -1383,8 +1383,11 @@ static int img_convert(int argc, char **argv) >>>> } >>>> } >>>> >>>> + ret = bdrv_get_info(out_bs, &bdi); >>>> + if (ret == 0) { >>>> + compress = compress || bdi.is_compressed; >>>> + } >>>> if (compress) { >>>> - ret = bdrv_get_info(out_bs, &bdi); >>>> if (ret < 0) { >>>> error_report("could not get block driver info"); >>>> goto out; >>> >>> You need to move the error checking up as well, above the >>> 'if (compress)' >>> >> >> Moving it up forces bdrv_get_info to succeed for both compress case >> and non compress case, which is too strict and fails when target >> driver doesn't support it. > > bdrv_get_info() will return -ENOTSUP if it is not supported. If we do > call down to the driver layer bdrv_get_info() and get an error, I > think we should pass it up. So just ignore -ENOTSUP if we don't care > about the case when bdrv_get_info() is not supported by a target. > We care for -ENOTSUP as well as other err code in the compressed case, and don't care the call to bdrv_get_info in the other case. I get your point, I'll add "else if (ret != -ENOTSUP) {...}" below the call, and keep the check of ret in "if (compressed)" unchanged. Fam
diff --git a/include/block/block.h b/include/block/block.h index 3560deb..169c092 100644 --- a/include/block/block.h +++ b/include/block/block.h @@ -18,6 +18,7 @@ typedef struct BlockDriverInfo { /* offset at which the VM state can be saved (0 if not possible) */ int64_t vm_state_offset; bool is_dirty; + bool is_compressed; } BlockDriverInfo; typedef struct BlockFragInfo { diff --git a/qemu-img.c b/qemu-img.c index bf3fb4f..09ed9b2 100644 --- a/qemu-img.c +++ b/qemu-img.c @@ -1383,8 +1383,11 @@ static int img_convert(int argc, char **argv) } } + ret = bdrv_get_info(out_bs, &bdi); + if (ret == 0) { + compress = compress || bdi.is_compressed; + } if (compress) { - ret = bdrv_get_info(out_bs, &bdi); if (ret < 0) { error_report("could not get block driver info"); goto out;
If target block driver forces compression, qemu-img convert needs to write by cluster size as well as "-c" option. Particularly, this applies for converting to VMDK streamOptimized format. Signed-off-by: Fam Zheng <famz@redhat.com> --- include/block/block.h | 1 + qemu-img.c | 5 ++++- 2 files changed, 5 insertions(+), 1 deletion(-)