[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v2 07/11] block: add x-blockdev-amend qmp command
From: |
Max Reitz |
Subject: |
Re: [PATCH v2 07/11] block: add x-blockdev-amend qmp command |
Date: |
Fri, 8 Nov 2019 11:36:51 +0100 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.1.1 |
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