Am 14.06.2016 um 23:30 hat Eric Blake geschrieben: > We want to eventually stick request_alignment alongside other > BlockLimits, but first, we must ensure it is populated at the > same time as all other limits, rather than being a special case > that is set only when a block is first opened. > > qemu-iotests 77 is particularly sensitive to the fact that we > can specify an artificial alignment override in blkdebug, and > that override must continue to work even when limits are > refreshed on an already open device. > > A later patch will be altering when bs->request_alignment > defaults are set, so fall back to sector alignment if we have > not inherited anything yet. > > Signed-off-by: Eric Blake <ebl...@redhat.com> > > --- > v2: new patch > --- > block/blkdebug.c | 18 +++++++++++++++--- > 1 file changed, 15 insertions(+), 3 deletions(-) > > diff --git a/block/blkdebug.c b/block/blkdebug.c > index 20d25bd..1589fa7 100644 > --- a/block/blkdebug.c > +++ b/block/blkdebug.c > @@ -37,6 +37,7 @@ > typedef struct BDRVBlkdebugState { > int state; > int new_state; > + int align; > > QLIST_HEAD(, BlkdebugRule) rules[BLKDBG__MAX]; > QSIMPLEQ_HEAD(, BlkdebugRule) active_rules; > @@ -382,9 +383,10 @@ static int blkdebug_open(BlockDriverState *bs, QDict > *options, int flags, > } > > /* Set request alignment */ > - align = qemu_opt_get_size(opts, "align", bs->request_alignment); > - if (align > 0 && align < INT_MAX && !(align & (align - 1))) { > - bs->request_alignment = align; > + align = qemu_opt_get_size(opts, "align", > + bs->request_alignment ?: BDRV_SECTOR_SIZE);
In the state as of this patch: How would bs->request_alignment ever be zero? It is always initialised as 512 (because blkdebug doesn't implement byte-based interfaces). Later in this series, you move the initialisation of the field to bdrv_refresh_limits(). However, the check still doesn't make sense because now it's always 0 and you always use the BDRV_SECTOR_SIZE fallback. I think you should either just unconditionally use BDRV_SECTOR_SIZE or even better just store 0 in s->align if the option isn't given. Your implementation of blkdebug_refresh_limits() already leaves the default bs->request_alignment alone if s->align == 0. > + if (align > 0 && align < INT_MAX && is_power_of_2(align)) { > + s->align = align; > } else { > error_setg(errp, "Invalid alignment"); > ret = -EINVAL; Kevin