On Wed 12 Oct 2016 04:30:27 PM CEST, Kevin Wolf <kw...@redhat.com> wrote: >> if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_STREAM, errp)) { >> goto out; >> } > > Added a bit more context. > > This check is redundant now... > >> if (has_base) { >> base_bs = bdrv_find_backing_image(bs, base); >> if (base_bs == NULL) { >> error_setg(errp, QERR_BASE_NOT_FOUND, base); >> goto out; >> } >> assert(bdrv_get_aio_context(base_bs) == aio_context); >> base_name = base; >> } >> >> + /* Check for op blockers in the whole chain between bs and base */ >> + for (iter = bs; iter && iter != base_bs; iter = backing_bs(iter)) { >> + if (bdrv_op_is_blocked(iter, BLOCK_OP_TYPE_STREAM, errp)) { >> + goto out; >> + } >> + } > > ...because you start with iter = bs here.
You're right, I'll fix it. > Another thought I had while looking at the previous few patches is > whether all the op blocker checks wouldn't be better moved to the > actual block job code (i.e. stream_start in this case). I thought about that too. In some cases I don't know if it's a good idea because the qmp_foo_bar() functions do a bit more than simply checking blockers (e.g. blockdev-mirror creates the target image), so you would want to have the checks before that. However doing it in the actual block job code could allow us to do other things. For example: at the moment when we call block-stream we check whether a number of BDSs are blocked (in qmp_block_stream()), and if they're not we proceed to block them (in stream_start()). We could make block_job_add_bdrv() do both things. On the other hand, since the plan is to move to a new block job API maybe it's better not to overdo things now. It's worth considering for the future anyway. Berto