| Message ID | 1487153430-17260-26-git-send-email-vsementsov@virtuozzo.com |
|---|---|
| State | New |
| Headers | show |
On 02/15/2017 05:10 AM, Vladimir Sementsov-Ogievskiy wrote: > Add detailed error messages. > > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> Reviewed-by: John Snow <jsnow@redhat.com> > --- > block/qcow2-bitmap.c | 48 ++++++++++++++++++++++++++++++++++-------------- > 1 file changed, 34 insertions(+), 14 deletions(-) > > diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c > index 9177c56..e25c872 100644 > --- a/block/qcow2-bitmap.c > +++ b/block/qcow2-bitmap.c > @@ -160,28 +160,49 @@ static int check_table_entry(uint64_t entry, int cluster_size) > > static int check_constraints_on_bitmap(BlockDriverState *bs, > const char *name, > - uint32_t granularity) > + uint32_t granularity, > + Error **errp) > { > BDRVQcow2State *s = bs->opaque; > int granularity_bits = ctz32(granularity); > int64_t len = bdrv_getlength(bs); > - bool fail; > > assert(granularity > 0); > assert((granularity & (granularity - 1)) == 0); > > if (len < 0) { > + error_setg_errno(errp, -len, "Failed to get size of '%s'", > + bdrv_get_device_or_node_name(bs)); > return len; > } > > - fail = (granularity_bits > BME_MAX_GRANULARITY_BITS) || > - (granularity_bits < BME_MIN_GRANULARITY_BITS) || > - (len > (uint64_t)BME_MAX_PHYS_SIZE << granularity_bits) || > - (len > (uint64_t)BME_MAX_TABLE_SIZE * s->cluster_size << > - granularity_bits) || > - (strlen(name) > BME_MAX_NAME_SIZE); > + if (granularity_bits > BME_MAX_GRANULARITY_BITS) { > + error_setg(errp, "Granularity exceeds maximum (%u bytes)", > + 1 << BME_MAX_GRANULARITY_BITS); > + return -EINVAL; > + } > + if (granularity_bits < BME_MIN_GRANULARITY_BITS) { > + error_setg(errp, "Granularity is under minimum (%u bytes)", > + 1 << BME_MIN_GRANULARITY_BITS); > + return -EINVAL; > + } > > - return fail ? -EINVAL : 0; > + if ((len > (uint64_t)BME_MAX_PHYS_SIZE << granularity_bits) || > + (len > (uint64_t)BME_MAX_TABLE_SIZE * s->cluster_size << > + granularity_bits)) > + { > + error_setg(errp, "Too much space will be occupied by the bitmap. " > + "Use larger granularity"); > + return -EINVAL; > + } > + > + if (strlen(name) > BME_MAX_NAME_SIZE) { > + error_setg(errp, "Name length exceeds maximum (%u characters)", > + BME_MAX_NAME_SIZE); > + return -EINVAL; > + } > + > + return 0; > } > > static void clear_bitmap_table(BlockDriverState *bs, uint64_t *bitmap_table, > @@ -1142,9 +1163,9 @@ void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp) > continue; > } > > - if (check_constraints_on_bitmap(bs, name, granularity) < 0) { > - error_setg(errp, "Bitmap '%s' doesn't satisfy the constraints", > - name); > + if (check_constraints_on_bitmap(bs, name, granularity, errp) < 0) { > + error_prepend(errp, "Bitmap '%s' doesn't satisfy the constraints: ", > + name); > goto fail; > } > > @@ -1233,8 +1254,7 @@ bool qcow2_can_store_new_dirty_bitmap(BlockDriverState *bs, > bool found; > Qcow2BitmapList *bm_list; > > - if (check_constraints_on_bitmap(bs, name, granularity) != 0) { > - error_setg(errp, "The constraints are not satisfied"); > + if (check_constraints_on_bitmap(bs, name, granularity, errp) != 0) { > goto fail; > } > >
Am 15.02.2017 um 11:10 hat Vladimir Sementsov-Ogievskiy geschrieben: > Add detailed error messages. > > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> Why not merge this patch into the one that originally introduced the function? Kevin
16.02.2017 17:21, Kevin Wolf wrote: > Am 15.02.2017 um 11:10 hat Vladimir Sementsov-Ogievskiy geschrieben: >> Add detailed error messages. >> >> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> > Why not merge this patch into the one that originally introduced the > function? Just to not create extra work for reviewers > > Kevin
On 02/17/2017 04:18 AM, Vladimir Sementsov-Ogievskiy wrote: > 16.02.2017 17:21, Kevin Wolf wrote: >> Am 15.02.2017 um 11:10 hat Vladimir Sementsov-Ogievskiy geschrieben: >>> Add detailed error messages. >>> >>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> >> Why not merge this patch into the one that originally introduced the >> function? > > Just to not create extra work for reviewers It's extra work for reviewers if you don't rebase obvious fixes where they belong - a new reviewer may flag the issue in the earlier patch only to find out later in the series that you've already fixed it. Avoiding needless code churn is part of what rebasing is all about - you want each step of the series to be self-contained and as correct as possible, by adding in the fixes at the point where they make sense, rather than at the end of the series.
17.02.2017 18:48, Eric Blake wrote: > On 02/17/2017 04:18 AM, Vladimir Sementsov-Ogievskiy wrote: >> 16.02.2017 17:21, Kevin Wolf wrote: >>> Am 15.02.2017 um 11:10 hat Vladimir Sementsov-Ogievskiy geschrieben: >>>> Add detailed error messages. >>>> >>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> >>> Why not merge this patch into the one that originally introduced the >>> function? >> Just to not create extra work for reviewers > It's extra work for reviewers if you don't rebase obvious fixes where > they belong - a new reviewer may flag the issue in the earlier patch > only to find out later in the series that you've already fixed it. > Avoiding needless code churn is part of what rebasing is all about - you > want each step of the series to be self-contained and as correct as > possible, by adding in the fixes at the point where they make sense, > rather than at the end of the series. > Ok, I'll rebase, It's not a problem.
diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c index 9177c56..e25c872 100644 --- a/block/qcow2-bitmap.c +++ b/block/qcow2-bitmap.c @@ -160,28 +160,49 @@ static int check_table_entry(uint64_t entry, int cluster_size) static int check_constraints_on_bitmap(BlockDriverState *bs, const char *name, - uint32_t granularity) + uint32_t granularity, + Error **errp) { BDRVQcow2State *s = bs->opaque; int granularity_bits = ctz32(granularity); int64_t len = bdrv_getlength(bs); - bool fail; assert(granularity > 0); assert((granularity & (granularity - 1)) == 0); if (len < 0) { + error_setg_errno(errp, -len, "Failed to get size of '%s'", + bdrv_get_device_or_node_name(bs)); return len; } - fail = (granularity_bits > BME_MAX_GRANULARITY_BITS) || - (granularity_bits < BME_MIN_GRANULARITY_BITS) || - (len > (uint64_t)BME_MAX_PHYS_SIZE << granularity_bits) || - (len > (uint64_t)BME_MAX_TABLE_SIZE * s->cluster_size << - granularity_bits) || - (strlen(name) > BME_MAX_NAME_SIZE); + if (granularity_bits > BME_MAX_GRANULARITY_BITS) { + error_setg(errp, "Granularity exceeds maximum (%u bytes)", + 1 << BME_MAX_GRANULARITY_BITS); + return -EINVAL; + } + if (granularity_bits < BME_MIN_GRANULARITY_BITS) { + error_setg(errp, "Granularity is under minimum (%u bytes)", + 1 << BME_MIN_GRANULARITY_BITS); + return -EINVAL; + } - return fail ? -EINVAL : 0; + if ((len > (uint64_t)BME_MAX_PHYS_SIZE << granularity_bits) || + (len > (uint64_t)BME_MAX_TABLE_SIZE * s->cluster_size << + granularity_bits)) + { + error_setg(errp, "Too much space will be occupied by the bitmap. " + "Use larger granularity"); + return -EINVAL; + } + + if (strlen(name) > BME_MAX_NAME_SIZE) { + error_setg(errp, "Name length exceeds maximum (%u characters)", + BME_MAX_NAME_SIZE); + return -EINVAL; + } + + return 0; } static void clear_bitmap_table(BlockDriverState *bs, uint64_t *bitmap_table, @@ -1142,9 +1163,9 @@ void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp) continue; } - if (check_constraints_on_bitmap(bs, name, granularity) < 0) { - error_setg(errp, "Bitmap '%s' doesn't satisfy the constraints", - name); + if (check_constraints_on_bitmap(bs, name, granularity, errp) < 0) { + error_prepend(errp, "Bitmap '%s' doesn't satisfy the constraints: ", + name); goto fail; } @@ -1233,8 +1254,7 @@ bool qcow2_can_store_new_dirty_bitmap(BlockDriverState *bs, bool found; Qcow2BitmapList *bm_list; - if (check_constraints_on_bitmap(bs, name, granularity) != 0) { - error_setg(errp, "The constraints are not satisfied"); + if (check_constraints_on_bitmap(bs, name, granularity, errp) != 0) { goto fail; }
Add detailed error messages. Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> --- block/qcow2-bitmap.c | 48 ++++++++++++++++++++++++++++++++++-------------- 1 file changed, 34 insertions(+), 14 deletions(-)