Re: [PATCH 4/6] block, migration: add bdrv_finalize_vmstate helper

[Markus Armbruster]

Daniel P. Berrangé <berrange@redhat.com> writes:

> On Thu, Aug 27, 2020 at 04:02:38PM +0300, Denis V. Lunev wrote:
>> On 8/27/20 3:58 PM, Daniel P. Berrangé wrote:
>> > On Thu, Jul 09, 2020 at 04:26:42PM +0300, Denis V. Lunev wrote:
>> >> Right now bdrv_fclose() is just calling bdrv_flush().
>> >>
>> >> The problem is that migration code is working inefficiently from block
>> >> layer terms and are frequently called for very small pieces of
>> >> unaligned data. Block layer is capable to work this way, but this is very
>> >> slow.
>> >>
>> >> This patch is a preparation for the introduction of the intermediate
>> >> buffer at block driver state. It would be beneficial to separate
>> >> conventional bdrv_flush() from closing QEMU file from migration code.
>> >>
>> >> The patch also forces bdrv_finalize_vmstate() operation inside
>> >> synchronous blk_save_vmstate() operation. This helper is used from
>> >> qemu-io only.
>> >>
>> >> Signed-off-by: Denis V. Lunev <den@openvz.org>
>> >> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> >> CC: Kevin Wolf <kwolf@redhat.com>
>> >> CC: Max Reitz <mreitz@redhat.com>
>> >> CC: Stefan Hajnoczi <stefanha@redhat.com>
>> >> CC: Fam Zheng <fam@euphon.net>
>> >> CC: Juan Quintela <quintela@redhat.com>
>> >> CC: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
>> >> CC: Denis Plotnikov <dplotnikov@virtuozzo.com>
>> >> ---
>> >>  block/block-backend.c |  6 +++++-
>> >>  block/io.c            | 15 +++++++++++++++
>> >>  include/block/block.h |  5 +++++
>> >>  migration/savevm.c    |  4 ++++
>> >>  4 files changed, 29 insertions(+), 1 deletion(-)
>> >> diff --git a/migration/savevm.c b/migration/savevm.c
>> >> index 45c9dd9d8a..d8a94e312c 100644
>> >> --- a/migration/savevm.c
>> >> +++ b/migration/savevm.c
>> >> @@ -150,6 +150,10 @@ static ssize_t block_get_buffer(void *opaque, 
>> >> uint8_t *buf, int64_t pos,
>> >>  
>> >>  static int bdrv_fclose(void *opaque, Error **errp)
>> >>  {
>> >> +    int err = bdrv_finalize_vmstate(opaque);
>> >> +    if (err < 0) {
>> >> +        return err;
>> > This is returning an error without having populating 'errp' which means
>> > the caller will be missing error diagnosis
>> 
>> but this behaves exactly like the branch below,
>> bdrv_flush() could return error too and errp
>> is not filled in the same way.
>
> Doh, it seems the only caller passes NULL for the errp too,
> so it is a redundant parameter. So nothing wrong with your
> patch after all.

Not setting an error on failure is plainly wrong.

If it works because all callers pass NULL, then the obvious fix is to
drop the @errp parameter.

I agree it's not this patch's fault.  It needs fixing anyway.  If you
have to respin for some other reason, including a fix would be nice.