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 <mre...@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
signature.asc
Description: OpenPGP digital signature
