| Message ID | 1408215258-12545-3-git-send-email-mreitz@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 08/16/2014 12:54 PM, Max Reitz wrote: > bdrv_is_allocated() may report zero clusters which most probably means > the image (file) is shorter than expected. Respect this case in order to > avoid an infinite loop. > > Signed-off-by: Max Reitz <mreitz@redhat.com> > --- > qemu-io-cmds.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > Reviewed-by: Eric Blake <eblake@redhat.com>
> + } else if (!num) { > + error_report("Unexpected end of image"); > + return 0; I think this test can miss some case of Unexpected end of image. For example supose that in map_is_allocated the first bdrv_is_allocated actually succeed then *pnum = num. Then the bottom loop has exit on failure and the function return. in map_f &num is map_is_allocated *pnum so map_f's num != 0 and this very test fails to see the unexpected end of image error. Best regards Benoît > } > > retstr = ret ? " allocated" : "not allocated"; > -- > 2.0.4 > >
Am 10.10.2014 um 14:03 schrieb Benoît Canet: >> + } else if (!num) { >> + error_report("Unexpected end of image"); >> + return 0; > I think this test can miss some case of Unexpected end of image. > > For example supose that in map_is_allocated the first bdrv_is_allocated > actually succeed then *pnum = num. Then the bottom loop has exit on failure > and the function return. > > in map_f &num is map_is_allocated *pnum so map_f's num != 0 and this very test > fails to see the unexpected end of image error. num != 0 because some sectors where successfully queried. In my opinion, we should print the information about them we have. Then, the do-while loop is repeated; and this time, map_is_allocated() either again returns num > 0 (for whatever reason, but I'd be fine with it) or, more probably, it'll be num == 0 this time. So the error is not missed, it's just printed one iteration later. Max > Best regards > > Benoît > >> } >> >> retstr = ret ? " allocated" : "not allocated"; >> -- >> 2.0.4 >> >>
The Saturday 11 Oct 2014 à 11:53:40 (+0200), Max Reitz wrote : > Am 10.10.2014 um 14:03 schrieb Benoît Canet: > >>+ } else if (!num) { > >>+ error_report("Unexpected end of image"); > >>+ return 0; > >I think this test can miss some case of Unexpected end of image. > > > >For example supose that in map_is_allocated the first bdrv_is_allocated > >actually succeed then *pnum = num. Then the bottom loop has exit on failure > >and the function return. > > > >in map_f &num is map_is_allocated *pnum so map_f's num != 0 and this very test > >fails to see the unexpected end of image error. > > num != 0 because some sectors where successfully queried. In my opinion, we > should print the information about them we have. Then, the do-while loop is > repeated; and this time, map_is_allocated() either again returns num > 0 > (for whatever reason, but I'd be fine with it) or, more probably, it'll be > num == 0 this time. So the error is not missed, it's just printed one > iteration later. Ok that make sense. Best regards Benoît > > Max > > >Best regards > > > >Benoît > > > >> } > >> retstr = ret ? " allocated" : "not allocated"; > >>-- > >>2.0.4 > >> > >> > >
The Saturday 16 Aug 2014 à 20:54:17 (+0200), Max Reitz wrote : > bdrv_is_allocated() may report zero clusters which most probably means > the image (file) is shorter than expected. Respect this case in order to > avoid an infinite loop. > > Signed-off-by: Max Reitz <mreitz@redhat.com> > --- > qemu-io-cmds.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/qemu-io-cmds.c b/qemu-io-cmds.c > index c503fc6..860b834 100644 > --- a/qemu-io-cmds.c > +++ b/qemu-io-cmds.c > @@ -1909,7 +1909,7 @@ static int map_is_allocated(BlockDriverState *bs, int64_t sector_num, > > num_checked = MIN(nb_sectors, INT_MAX); > ret = bdrv_is_allocated(bs, sector_num, num_checked, &num); > - if (ret == firstret) { > + if (ret == firstret && num) { > *pnum += num; > } else { > break; > @@ -1936,6 +1936,9 @@ static int map_f(BlockDriverState *bs, int argc, char **argv) > if (ret < 0) { > error_report("Failed to get allocation status: %s", strerror(-ret)); > return 0; > + } else if (!num) { > + error_report("Unexpected end of image"); > + return 0; > } > > retstr = ret ? " allocated" : "not allocated"; > -- > 2.0.4 > > Reviewed-by: Benoît Canet <benoit.canet@nodalink.com>
diff --git a/qemu-io-cmds.c b/qemu-io-cmds.c index c503fc6..860b834 100644 --- a/qemu-io-cmds.c +++ b/qemu-io-cmds.c @@ -1909,7 +1909,7 @@ static int map_is_allocated(BlockDriverState *bs, int64_t sector_num, num_checked = MIN(nb_sectors, INT_MAX); ret = bdrv_is_allocated(bs, sector_num, num_checked, &num); - if (ret == firstret) { + if (ret == firstret && num) { *pnum += num; } else { break; @@ -1936,6 +1936,9 @@ static int map_f(BlockDriverState *bs, int argc, char **argv) if (ret < 0) { error_report("Failed to get allocation status: %s", strerror(-ret)); return 0; + } else if (!num) { + error_report("Unexpected end of image"); + return 0; } retstr = ret ? " allocated" : "not allocated";
bdrv_is_allocated() may report zero clusters which most probably means the image (file) is shorter than expected. Respect this case in order to avoid an infinite loop. Signed-off-by: Max Reitz <mreitz@redhat.com> --- qemu-io-cmds.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)