13.04.2019 19:53, Max Reitz wrote: > This new error option allows users of blkdebug to inject errors only on > certain kinds of I/O operations. Users usually want to make a very > specific operation fail, not just any; but right now they simply hope > that the event that triggers the error injection is followed up with > that very operation. That may not be true, however, because the block > layer is changing (including blkdebug, which may increase the number of > types of I/O operations on which to inject errors). > > The new option's default has been chosen to keep backwards > compatibility. > > Note that similar to the internal representation, we could choose to > expose this option as a list of I/O types. But there is no practical > use for this, because as described above, users usually know exactly > which kind of operation they want to make fail, so there is no need to > specify multiple I/O types at once. In addition, exposing this option > as a list would require non-trivial changes to qemu_opts_absorb_qdict(). > > Signed-off-by: Max Reitz <mre...@redhat.com> > --- > qapi/block-core.json | 26 +++++++++++++++++++++++ > block/blkdebug.c | 50 ++++++++++++++++++++++++++++++++++++-------- > 2 files changed, 67 insertions(+), 9 deletions(-) > > diff --git a/qapi/block-core.json b/qapi/block-core.json > index 7ccbfff9d0..5141e91172 100644 > --- a/qapi/block-core.json > +++ b/qapi/block-core.json > @@ -3235,6 +3235,26 @@ > 'l1_shrink_write_table', 'l1_shrink_free_l2_clusters', > 'cor_write'] } > > +## > +# @BlkdebugIOType: > +# > +# Kinds of I/O that blkdebug can inject errors in. > +# > +# @read: .bdrv_co_preadv() > +# > +# @write: .bdrv_co_pwritev() > +# > +# @write-zeroes: .bdrv_co_pwrite_zeroes() > +# > +# @discard: .bdrv_co_pdiscard() > +# > +# @flush: .bdrv_co_flush_to_disk() > +# > +# Since: 4.1 > +## > +{ 'enum': 'BlkdebugIOType', > + 'data': [ 'read', 'write', 'write-zeroes', 'discard', 'flush' ] } > + > ## > # @BlkdebugInjectErrorOptions: > # > @@ -3245,6 +3265,11 @@ > # @state: the state identifier blkdebug needs to be in to > # actually trigger the event; defaults to "any" > # > +# @iotype: the type of I/O operations on which this error should > +# be injected; defaults to "all read, write, > +# write-zeroes, discard, and flush operations"
Don't you want defaults to any? > +# (since: 4.1) > +# > # @errno: error identifier (errno) to be returned; defaults to > # EIO > # > @@ -3262,6 +3287,7 @@ > { 'struct': 'BlkdebugInjectErrorOptions', > 'data': { 'event': 'BlkdebugEvent', > '*state': 'int', > + '*iotype': 'BlkdebugIOType', > '*errno': 'int', > '*sector': 'int', > '*once': 'bool', > diff --git a/block/blkdebug.c b/block/blkdebug.c > index efd9441625..ca84b12e32 100644 > --- a/block/blkdebug.c > +++ b/block/blkdebug.c > @@ -75,6 +75,7 @@ typedef struct BlkdebugRule { > int state; > union { > struct { > + uint64_t iotype_mask; > int error; > int immediately; > int once; > @@ -91,6 +92,9 @@ typedef struct BlkdebugRule { > QSIMPLEQ_ENTRY(BlkdebugRule) active_next; > } BlkdebugRule; > > +QEMU_BUILD_BUG_MSG(BLKDEBUGIO_TYPE__MAX > 64, > + "BlkdebugIOType mask does not fit into an uint64_t"); > + > static QemuOptsList inject_error_opts = { > .name = "inject-error", > .head = QTAILQ_HEAD_INITIALIZER(inject_error_opts.head), > @@ -103,6 +107,10 @@ static QemuOptsList inject_error_opts = { > .name = "state", > .type = QEMU_OPT_NUMBER, > }, > + { > + .name = "iotype", > + .type = QEMU_OPT_STRING, > + }, > { > .name = "errno", > .type = QEMU_OPT_NUMBER, > @@ -162,6 +170,8 @@ static int add_rule(void *opaque, QemuOpts *opts, Error > **errp) > int event; > struct BlkdebugRule *rule; > int64_t sector; > + BlkdebugIOType iotype; > + Error *local_error = NULL; > > /* Find the right event for the rule */ > event_name = qemu_opt_get(opts, "event"); > @@ -192,6 +202,26 @@ static int add_rule(void *opaque, QemuOpts *opts, Error > **errp) > sector = qemu_opt_get_number(opts, "sector", -1); > rule->options.inject.offset = > sector == -1 ? -1 : sector * BDRV_SECTOR_SIZE; > + > + iotype = qapi_enum_parse(&BlkdebugIOType_lookup, > + qemu_opt_get(opts, "iotype"), > + BLKDEBUGIO_TYPE__MAX, &local_error); Prefix BLKDEBUGIO seems strange. Looks like a bug in qapi, I think it should make BLKDEBUG prefix in this case. Don't you want use "'prefix': 'BLKDBG'" like it is done for blkdebug events? > + if (local_error) { > + error_propagate(errp, local_error); > + return -1; > + } > + if (iotype != BLKDEBUGIO_TYPE__MAX) { > + rule->options.inject.iotype_mask = (1ull << iotype); > + } else { > + /* Apply the default */ > + rule->options.inject.iotype_mask = > + (1ull << BLKDEBUGIO_TYPE_READ) > + | (1ull << BLKDEBUGIO_TYPE_WRITE) > + | (1ull << BLKDEBUGIO_TYPE_WRITE_ZEROES) > + | (1ull << BLKDEBUGIO_TYPE_DISCARD) > + | (1ull << BLKDEBUGIO_TYPE_FLUSH); and here (1ull << BLKDEBUGIO_TYPE__MAX) - 1 ? [..] It is my first my first look at blkdebug internals, but patch seems OK for me, so, with or without my tiny suggestions: Reviewed-by: Vladimir Sementsov-Ogievskiy <vsement...@virtuozzo.com> -- Best regards, Vladimir