Re: [Qemu-block] [PATCH v10 09/16] block: Add QMP support for streaming to an intermediate layer

[Alberto Garcia]

On Wed 12 Oct 2016 04:30:27 PM CEST, Kevin Wolf <address@hidden> 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