[Qemu-devel] [PATCH] block:Fix FIXED base vpc be probed to raw format

[Qemu-devel] [PATCH] block:Fix FIXED base vpc be probed to raw format

[Xiaodong Gong]

When open the vpc snapshot based FIXED format, its backing file,
this FIXED vpc image, could be probed as a raw image, because that
the find_image_format just checkout the first sector.

Add a re-probe for the last sector to FIXED base vpc image,when get
a NULL or raw driver in first sector probe.

Signed-off-by: Xiaodong Gong <gongxiaodong1@huawei.com>
---
 block.c | 55 ++++++++++++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 48 insertions(+), 7 deletions(-)

diff --git a/block.c b/block.c
index d45e4dd..e183515 100644
--- a/block.c
+++ b/block.c
@@ -702,20 +702,24 @@ BlockDriver *bdrv_probe_all(const uint8_t *buf, int buf_size,
     return drv;
 }
 
-static int find_image_format(BlockDriverState *bs, const char *filename,
-                             BlockDriver **pdrv, Error **errp)
+static int probe_image_format(BlockDriverState *bs, const char *filename,
+                              int64_t offset, BlockDriver **pdrv, Error **errp,
+                              bool isfoot)
 {
     BlockDriver *drv;
     uint8_t buf[BLOCK_PROBE_BUF_SIZE];
     int ret = 0;
 
-    /* Return the raw BlockDriver * to scsi-generic devices or empty drives */
-    if (bs->sg || !bdrv_is_inserted(bs) || bdrv_getlength(bs) == 0) {
-        *pdrv = &bdrv_raw;
+    if (!bs || !filename) {
+        *pdrv = NULL;
         return ret;
     }
 
-    ret = bdrv_pread(bs, 0, buf, sizeof(buf));
+    if (offset + sizeof(buf) > bs->total_sectors << BDRV_SECTOR_BITS) {
+        return ret;
+    }
+
+    ret = bdrv_pread(bs, offset, buf, sizeof(buf));
     if (ret < 0) {
         error_setg_errno(errp, -ret, "Could not read image for determining its "
                          "format");
@@ -724,12 +728,49 @@ static int find_image_format(BlockDriverState *bs, const char *filename,
     }
 
     drv = bdrv_probe_all(buf, ret, filename);
-    if (!drv) {
+    if (!drv && isfoot) {
         error_setg(errp, "Could not determine image format: No compatible "
                    "driver found");
         ret = -ENOENT;
     }
     *pdrv = drv;
+
+    return ret;
+}
+
+static int find_image_format(BlockDriverState *bs, const char *filename,
+                             BlockDriver **pdrv, Error **errp)
+{
+    int last_sector_offset;
+    int ret = 0;
+
+    *pdrv = NULL;
+
+    /* Return the raw BlockDriver * to scsi-generic devices or empty drives */
+    if (bs->sg || !bdrv_is_inserted(bs) || bdrv_getlength(bs) == 0) {
+        *pdrv = &bdrv_raw;
+        return ret;
+    }
+
+    /* probe the header to guess image format */
+    ret = probe_image_format(bs, filename, 0, pdrv, errp, false);
+    if (ret < 0) {
+        return ret;
+    }
+
+    /* The FIXED base img of VHD only has the footer in tail. The drv could
+     * be NULL or raw */
+    if (*pdrv && *pdrv != &bdrv_raw) {
+        return ret;
+    }
+
+    last_sector_offset = (bs->total_sectors - 1) << BDRV_SECTOR_BITS;
+    ret = probe_image_format(bs, filename, last_sector_offset, pdrv,
+        errp, true);
+    if (ret < 0) {
+        return ret;
+    }
+
     return ret;
 }
 
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH] block:Fix FIXED base vpc be probed to raw format

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 986 bytes --]

On Sun, Feb 08, 2015 at 08:39:52PM +0800, Xiaodong Gong wrote:
> When open the vpc snapshot based FIXED format, its backing file,
> this FIXED vpc image, could be probed as a raw image, because that
> the find_image_format just checkout the first sector.
> 
> Add a re-probe for the last sector to FIXED base vpc image,when get
> a NULL or raw driver in first sector probe.
> 
> Signed-off-by: Xiaodong Gong <gongxiaodong1@huawei.com>
> ---
>  block.c | 55 ++++++++++++++++++++++++++++++++++++++++++++++++-------
>  1 file changed, 48 insertions(+), 7 deletions(-)

This approach is likely to cause issues because it makes no distiction
between headers and footers.  Most image formats only have a header and
would now be misdetected if a raw image had a header in its last sector.

When a vpc image has a backing file, does it make sense to set the
bs->backing_format to "vpc"?  That way we'll try to open the backing
file as a vpc image without probing.

Stefan

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [PATCH] block:Fix FIXED base vpc be probed to raw format

[Xiaodong Gong]

[-- Attachment #1: Type: text/plain, Size: 1282 bytes --]

On Mon, Feb 9, 2015 at 10:54 PM, Stefan Hajnoczi <stefanha@gmail.com> wrote:

> On Sun, Feb 08, 2015 at 08:39:52PM +0800, Xiaodong Gong wrote:
> > When open the vpc snapshot based FIXED format, its backing file,
> > this FIXED vpc image, could be probed as a raw image, because that
> > the find_image_format just checkout the first sector.
> >
> > Add a re-probe for the last sector to FIXED base vpc image,when get
> > a NULL or raw driver in first sector probe.
> >
> > Signed-off-by: Xiaodong Gong <gongxiaodong1@huawei.com>
> > ---
> >  block.c | 55 ++++++++++++++++++++++++++++++++++++++++++++++++-------
> >  1 file changed, 48 insertions(+), 7 deletions(-)
>
> This approach is likely to cause issues because it makes no distiction
> between headers and footers.  Most image formats only have a header and
> would now be misdetected if a raw image had a header in its last sector.
>
> When a vpc image has a backing file, does it make sense to set the
> bs->backing_format to "vpc"?  That way we'll try to open the backing
> file as a vpc image without probing.
>
> Stefan
>


It works, but this probing of format sounds good . Giving a argument of bs
to bdrv_probe and each of funtion of probe get it's own checking method is
better, but the foot-print of patch is longer.

[-- Attachment #2: Type: text/html, Size: 1992 bytes --]

Re: [Qemu-devel] [PATCH] block:Fix FIXED base vpc be probed to raw format

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1638 bytes --]

On Tue, Feb 10, 2015 at 11:04:31PM +0800, Xiaodong Gong wrote:
> On Mon, Feb 9, 2015 at 10:54 PM, Stefan Hajnoczi <stefanha@gmail.com> wrote:
> 
> > On Sun, Feb 08, 2015 at 08:39:52PM +0800, Xiaodong Gong wrote:
> > > When open the vpc snapshot based FIXED format, its backing file,
> > > this FIXED vpc image, could be probed as a raw image, because that
> > > the find_image_format just checkout the first sector.
> > >
> > > Add a re-probe for the last sector to FIXED base vpc image,when get
> > > a NULL or raw driver in first sector probe.
> > >
> > > Signed-off-by: Xiaodong Gong <gongxiaodong1@huawei.com>
> > > ---
> > >  block.c | 55 ++++++++++++++++++++++++++++++++++++++++++++++++-------
> > >  1 file changed, 48 insertions(+), 7 deletions(-)
> >
> > This approach is likely to cause issues because it makes no distiction
> > between headers and footers.  Most image formats only have a header and
> > would now be misdetected if a raw image had a header in its last sector.
> >
> > When a vpc image has a backing file, does it make sense to set the
> > bs->backing_format to "vpc"?  That way we'll try to open the backing
> > file as a vpc image without probing.
> >
> > Stefan
> >
> 
> 
> It works, but this probing of format sounds good . Giving a argument of bs
> to bdrv_probe and each of funtion of probe get it's own checking method is
> better, but the foot-print of patch is longer.

I'm not suggesting a longer patch.

Can the vpc .bdrv_open() code that loads the backing filename also set
bs->backing_format to "vpc" so QEMU will always use vpc for the backing
file?

Stefan

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [PATCH] block:Fix FIXED base vpc be probed to raw format

[Xiaodong Gong]

[-- Attachment #1: Type: text/plain, Size: 1833 bytes --]

On Wed, Feb 11, 2015 at 9:39 PM, Stefan Hajnoczi <stefanha@redhat.com>
wrote:

> On Tue, Feb 10, 2015 at 11:04:31PM +0800, Xiaodong Gong wrote:
> > On Mon, Feb 9, 2015 at 10:54 PM, Stefan Hajnoczi <stefanha@gmail.com>
> wrote:
> >
> > > On Sun, Feb 08, 2015 at 08:39:52PM +0800, Xiaodong Gong wrote:
> > > > When open the vpc snapshot based FIXED format, its backing file,
> > > > this FIXED vpc image, could be probed as a raw image, because that
> > > > the find_image_format just checkout the first sector.
> > > >
> > > > Add a re-probe for the last sector to FIXED base vpc image,when get
> > > > a NULL or raw driver in first sector probe.
> > > >
> > > > Signed-off-by: Xiaodong Gong <gongxiaodong1@huawei.com>
> > > > ---
> > > >  block.c | 55 ++++++++++++++++++++++++++++++++++++++++++++++++-------
> > > >  1 file changed, 48 insertions(+), 7 deletions(-)
> > >
> > > This approach is likely to cause issues because it makes no distiction
> > > between headers and footers.  Most image formats only have a header and
> > > would now be misdetected if a raw image had a header in its last
> sector.
> > >
> > > When a vpc image has a backing file, does it make sense to set the
> > > bs->backing_format to "vpc"?  That way we'll try to open the backing
> > > file as a vpc image without probing.
> > >
> > > Stefan
> > >
> >
> >
> > It works, but this probing of format sounds good . Giving a argument of
> bs
> > to bdrv_probe and each of funtion of probe get it's own checking method
> is
> > better, but the foot-print of patch is longer.
>
> I'm not suggesting a longer patch.
>
> Can the vpc .bdrv_open() code that loads the backing filename also set
> bs->backing_format to "vpc" so QEMU will always use vpc for the backing
> file?
>
> Stefan
>

It is not a perfect choice, but a right one. I will give a v2 of patch.

[-- Attachment #2: Type: text/html, Size: 2709 bytes --]

Re: [Qemu-devel] [PATCH] block:Fix FIXED base vpc be probed to raw format

[Xiaodong Gong]

[-- Attachment #1: Type: text/plain, Size: 2104 bytes --]

Sent the v2 version of this patch integrated in v9 verison of VHD_DEFFERING
patch.
Fixed this problem with backing_format.

On Sat, Feb 14, 2015 at 10:13 AM, Xiaodong Gong <gordongong0350@gmail.com>
wrote:

>
>
> On Wed, Feb 11, 2015 at 9:39 PM, Stefan Hajnoczi <stefanha@redhat.com>
> wrote:
>
>> On Tue, Feb 10, 2015 at 11:04:31PM +0800, Xiaodong Gong wrote:
>> > On Mon, Feb 9, 2015 at 10:54 PM, Stefan Hajnoczi <stefanha@gmail.com>
>> wrote:
>> >
>> > > On Sun, Feb 08, 2015 at 08:39:52PM +0800, Xiaodong Gong wrote:
>> > > > When open the vpc snapshot based FIXED format, its backing file,
>> > > > this FIXED vpc image, could be probed as a raw image, because that
>> > > > the find_image_format just checkout the first sector.
>> > > >
>> > > > Add a re-probe for the last sector to FIXED base vpc image,when get
>> > > > a NULL or raw driver in first sector probe.
>> > > >
>> > > > Signed-off-by: Xiaodong Gong <gongxiaodong1@huawei.com>
>> > > > ---
>> > > >  block.c | 55
>> ++++++++++++++++++++++++++++++++++++++++++++++++-------
>> > > >  1 file changed, 48 insertions(+), 7 deletions(-)
>> > >
>> > > This approach is likely to cause issues because it makes no distiction
>> > > between headers and footers.  Most image formats only have a header
>> and
>> > > would now be misdetected if a raw image had a header in its last
>> sector.
>> > >
>> > > When a vpc image has a backing file, does it make sense to set the
>> > > bs->backing_format to "vpc"?  That way we'll try to open the backing
>> > > file as a vpc image without probing.
>> > >
>> > > Stefan
>> > >
>> >
>> >
>> > It works, but this probing of format sounds good . Giving a argument of
>> bs
>> > to bdrv_probe and each of funtion of probe get it's own checking method
>> is
>> > better, but the foot-print of patch is longer.
>>
>> I'm not suggesting a longer patch.
>>
>> Can the vpc .bdrv_open() code that loads the backing filename also set
>> bs->backing_format to "vpc" so QEMU will always use vpc for the backing
>> file?
>>
>> Stefan
>>
>
> It is not a perfect choice, but a right one. I will give a v2 of patch.
>

[-- Attachment #2: Type: text/html, Size: 3252 bytes --]