Am 06.03.2018 um 16:01 hat Alberto Garcia geschrieben:
> On Tue 06 Mar 2018 03:54:26 PM CET, Kevin Wolf wrote:
> >> @@ -2092,11 +2092,18 @@ int qcow2_expand_zero_clusters(BlockDriverState
> >> *bs,
> >> }
> >>
> >> for (i = 0; i < s->nb_snapshots; i++) {
> >> - int l1_sectors = DIV_ROUND_UP(s->snapshots[i].l1_size *
> >> - sizeof(uint64_t), BDRV_SECTOR_SIZE);
> >> + int l1_size2;
> >> + uint64_t *new_l1_table;
> >>
> >> - uint64_t *new_l1_table =
> >> - g_try_realloc(l1_table, l1_sectors * BDRV_SECTOR_SIZE);
> >> + ret = qcow2_validate_table(bs, s->snapshots[i].l1_table_offset,
> >> + s->snapshots[i].l1_size,
> >> sizeof(uint64_t),
> >> + QCOW_MAX_L1_SIZE, "", NULL);
> >> + if (ret < 0) {
> >> + return ret;
> >
> > Shouldn't this be goto fail?
>
> You're right, this is a loop, and l1_table could have been initialized
> in previous iterations.
>
> I'll send a corrected version with this change, but first I'll wait a
> bit in case you see anything else in the series.
I've finished the review now, the rest looks correct.
The only other thing I wondered is about the cases where you pass a
NULL errp because the callers don't get an Error parameter, so they
can't pass it on. Some of these callers already use error_report(), so
it would be okay to use error_report_err() for an error returned by
qcow2_validate_table(), too. I think that would improve the messages.
Kevin
