Re: [PATCH v2 07/11] block: add x-blockdev-amend qmp command

[Max Reitz]

On 08.11.19 10:26, Maxim Levitsky wrote:
> On Fri, 2019-10-04 at 20:53 +0200, Max Reitz wrote:
>> On 13.09.19 00:30, Maxim Levitsky wrote:
>>> Signed-off-by: Maxim Levitsky <address@hidden>
>>> ---
>>>  block/Makefile.objs       |   2 +-
>>>  block/amend.c             | 116 ++++++++++++++++++++++++++++++++++++++
>>>  include/block/block_int.h |  23 ++++++--
>>>  qapi/block-core.json      |  26 +++++++++
>>>  qapi/job.json             |   4 +-
>>>  5 files changed, 163 insertions(+), 8 deletions(-)
>>>  create mode 100644 block/amend.c

[...]

>>> +static int coroutine_fn blockdev_amend_run(Job *job, Error **errp)
>>> +{
>>> +    BlockdevAmendJob *s = container_of(job, BlockdevAmendJob, common);
>>> +    int ret;
>>> +
>>> +    job_progress_set_remaining(&s->common, 1);
>>> +    ret = s->bs->drv->bdrv_co_amend(s->bs, s->opts, s->force, errp);
>>> +    job_progress_update(&s->common, 1);
>>
>> It would be nice if the amend job could make use of the progress
>> reporting that we have in place for amend.
> 
> I also thought about it, but is it worth it?
> 
> I looked through the status reporting of the qcow2 amend
> code (which doesn't really allowed to be run through
> qmp blockdev-amend, due to complexity of changing 
> the qcow2 format on the fly).

True, and we could always add it later.

I suppose I was mostly wondering because bdrv_amend_options already has
all of that infrastructure and I was assuming that qcow2's bdrv_co_amend
implementation would make some use of the existing function.  Well, it
doesn’t, so *shrug*

[...]

>>> +    /*
>>> +     * Create the block job
>>> +     * TODO Running in the main context. Block drivers need to error out 
>>> or add
>>> +     * locking when they use a BDS in a different AioContext.
>>
>> Why shouldn’t the job just run in the node’s context?
> 
> This is shameless copy&pasta from the blockdev-create code
> (which I did note in the copyright of the file)
Well, you noted that it’s heavily based on it, not that it’s just C&P.

So I suppose the comment is just wrong here?

Max

signature.asc
Description: OpenPGP digital signature