[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v3 06/15] block/mirror: conservative mirror_exit
From: |
John Snow |
Subject: |
Re: [Qemu-devel] [PATCH v3 06/15] block/mirror: conservative mirror_exit refactor |
Date: |
Tue, 4 Sep 2018 12:17:39 -0400 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.8.0 |
On 09/03/2018 05:32 AM, Max Reitz wrote:
> On 2018-09-01 00:28, John Snow wrote:
>> For purposes of minimum code movement, refactor the mirror_exit
>> callback to use the post-finalization callbacks in a trivial way.
>>
>> Signed-off-by: John Snow <address@hidden>
>> ---
>> block/mirror.c | 31 +++++++++++++++++++++++++------
>> 1 file changed, 25 insertions(+), 6 deletions(-)
>>
>> diff --git a/block/mirror.c b/block/mirror.c
>> index c164fee883..5067f1764d 100644
>> --- a/block/mirror.c
>> +++ b/block/mirror.c
>
> [...]
>
>> @@ -617,7 +618,13 @@ static void mirror_exit(Job *job)
>> BlockDriverState *target_bs = blk_bs(s->target);
>> BlockDriverState *mirror_top_bs = s->mirror_top_bs;
>> Error *local_err = NULL;
>> - int ret = job->ret;
>> + bool abort = !!job->ret;
>
> I think "job->ret < 0" could be read more easily.
Hm, OK. We shouldn't be using > 0 retcodes anyway.
>
>> + int ret = 0;
>> +
>> + if (s->prepared) {
>> + return 0;
>> + }
>> + s->prepared = true;
>>
>> bdrv_release_dirty_bitmap(src, s->dirty_bitmap);
>>
>
> [...]
>
>> @@ -712,7 +719,17 @@ static void mirror_exit(Job *job)
>> bdrv_unref(mirror_top_bs);
>> bdrv_unref(src);
>>
>> - job->ret = ret;
>> + return ret;
>> +}
>> +
>> +static int mirror_prepare(Job *job)
>> +{
>> + return mirror_exit_common(job);
>> +}
>> +
>> +static void mirror_abort(Job *job)
>> +{
>> + assert(mirror_exit_common(job) == 0);
>
> You shouldn't execute vital code in assert(), as NDEBUG would make that
> code disappear. As far as I know, we have decided (at some point) not
> to ever enable NDEBUG in qemu, but, you know. Doing it right only costs
> one more line, and it would get you a
>
d'oh.
> Reviewed-by: Max Reitz <address@hidden>
>
>> }
>>
>> static void mirror_throttle(MirrorBlockJob *s)
>