Daniel P. Berrangé <[email protected]> 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 <[email protected]> >> >> Reviewed-by: Vladimir Sementsov-Ogievskiy <[email protected]> >> >> CC: Kevin Wolf <[email protected]> >> >> CC: Max Reitz <[email protected]> >> >> CC: Stefan Hajnoczi <[email protected]> >> >> CC: Fam Zheng <[email protected]> >> >> CC: Juan Quintela <[email protected]> >> >> CC: "Dr. David Alan Gilbert" <[email protected]> >> >> CC: Denis Plotnikov <[email protected]> >> >> --- >> >> 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.
