Re: [Qemu-devel] [PATCH v4 04/21] stream: Drop reached_end for stream_complete()

[John Snow <jsnow@redhat.com>]

On 07/05/2017 05:08 PM, Eric Blake wrote:
> stream_complete() skips the work of rewriting the backing file if
> the job was cancelled, if data->reached_end is false, or if there
> was an error detected (non-zero data->ret) during the streaming.
> But note that in stream_run(), data->reached_end is only set if the
> loop ran to completion, and data->ret is only 0 in two cases:
> either the loop ran to completion (possibly by cancellation, but
> stream_complete checks for that), or we took an early goto out
> because there is no bs->backing.  Thus, we can preserve the same
> semantics without the use of reached_end, by merely checking for
> bs->backing (and logically, if there was no backing file, streaming
> is a no-op, so there is no backing file to rewrite).
> 
> Suggested-by: Kevin Wolf <kwolf@redhat.com>
> Signed-off-by: Eric Blake <eblake@redhat.com>
> 
> ---
> v4: new patch
> ---
>  block/stream.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/block/stream.c b/block/stream.c
> index 746d525..12f1659 100644
> --- a/block/stream.c
> +++ b/block/stream.c
> @@ -59,7 +59,6 @@ static int coroutine_fn stream_populate(BlockBackend *blk,
> 
>  typedef struct {
>      int ret;
> -    bool reached_end;
>  } StreamCompleteData;
> 
>  static void stream_complete(BlockJob *job, void *opaque)
> @@ -70,7 +69,7 @@ static void stream_complete(BlockJob *job, void *opaque)
>      BlockDriverState *base = s->base;
>      Error *local_err = NULL;
> 
> -    if (!block_job_is_cancelled(&s->common) && data->reached_end &&
> +    if (!block_job_is_cancelled(&s->common) && bs->backing &&
>          data->ret == 0) {
>          const char *base_id = NULL, *base_fmt = NULL;
>          if (base) {
> @@ -211,7 +210,6 @@ out:
>      /* Modify backing chain and close BDSes in main loop */
>      data = g_malloc(sizeof(*data));
>      data->ret = ret;
> -    data->reached_end = sector_num == end;
>      block_job_defer_to_main_loop(&s->common, stream_complete, data);
>  }
> 

This seems ever so slightly less intuitive to me, but it is functionally
identical.

Reviewed-by: John Snow <jsnow@redhat.com>