Re: [Qemu-devel] [PATCH] block/stream: Don't stream unbacked devices

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 2216 bytes --]

On 11/14/2013 06:41 AM, Stefan Hajnoczi wrote:
> On Wed, Nov 13, 2013 at 08:37:58PM +0100, Max Reitz wrote:
>> If a block device is unbacked, a streaming blockjob should immediately
>> finish instead of beginning to try to stream, then noticing the backing
>> file does not contain even the first sector (since it does not exist)
>> and then finishing normally.
>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>>  block/stream.c | 5 +++++
>>  1 file changed, 5 insertions(+)
>>
>> diff --git a/block/stream.c b/block/stream.c
>> index 694fd42..46bec7d 100644
>> --- a/block/stream.c
>> +++ b/block/stream.c
>> @@ -88,6 +88,11 @@ static void coroutine_fn stream_run(void *opaque)
>>      int n = 0;
>>      void *buf;
>>  
>> +    if (!bs->backing_hd) {
>> +        block_job_completed(&s->common, 0);
>> +        return;
>> +    }
>> +
> 
> Thanks for raising this, it's a bug that we don't verify that the image
> has a backing file.
> 
> I'd rather return an error that the user attempted to do something
> pointless.  It was a mistake on their part and it helps to bring this to
> their attention right away.

Conversely, we can argue that streaming a file that has no backing
should SUCCEED as a no-op (there's nothing further to stream, so we are
done right away).

> 
> CCing Eric Blake because strictly speaking, adding a error where we
> previously used to return success breaks API compatibility.  Any
> suggestions (I guess QEMU could just log a warning instead of failing
> the command)?

Turning this into an error would be a regression, and should not be
done.  Based on previous behavior, we have to keep it successful.  But
if I'm reading the patch correctly, it appears that all you are trying
to do is optimize the no-op case to be even more of a no-op (faster
success return, rather than setting up a lot of structure just to find
no further work to do), and not turn a previous success case into an
error.  Adding a log message about a suspicious may make sense, but if
there is no error, will anyone think to check the log?

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]