Re: [Qemu-devel] 'commit' error for 'all': No medium found

[Markus Armbruster]

Jan Kiszka <address@hidden> writes:

> On 2013-02-25 14:05, Jeff Cody wrote:
>> On Sun, Feb 24, 2013 at 07:29:31PM +0100, Jan Kiszka wrote:
>>> Hi,
>>>
>>> I'm seeing this with git head and 1.4. Apparently, commit on a
>>> non-populated medium now generates this error instead of ignoring it
>>> like in the past. As we stop iterating over the block devices while
>>> doing "all", this may leaving uncommitted data behind.
>>>
>>> Didn't test, but I suspect 58513bde83 has something to do with it.
>>>
>>> Jan
>>>
>> 
>> Hi Jan,
>> 
>> That commit just affected the reporting on the error - for the "all"
>> case, bdrv_commit_all() still returned error on the first failure.
>> When that happened 'commit' may have indicated success rather than
>> error, depending on the error.
>
> Sorry, I just picked on the first commit that jumped into my eyes.
>
>> 
>> That would have also left uncommitted data behind, but done so
>> silently and reported success.
>> 
>> However, commit e8877497 added error checking to the bdrv_commit()
>> return value in bdrv_commit_all() - before that bdrv_commit_all()
>> ignored all error returns of bdrv_commit().
>> 
>> Do you think there specific error returns that we should ignore then, in
>> bdrv_commit_all(), such as -ENOMEDIUM?
>
> I think commit on a device without a medium should be a nop (as the

Yes.

> commit backlog is empty - provided it's correctly dropped on eject).

Monitor commands eject and change close the backend.  This does not
collapse ("commit") any COWs into their backing images.  If the backend
was opened with BDRV_O_SNAPSHOT, the uncommitted updates are lost.
That's a feature.

>> Also, could you expand on what you mean by non-populated
>> medium (test case) - is the error being returned "No medium found"?
>
> This is a default PC setup: the floppy tends to be not injected, thus
> "commit all" now reports this error. If there is a blockdev after the
> floppy in our list, it will not be committed this way. Luckly, the HD is
> queued before the FD.

Making bdrv_commit_all() check errors was a good idea, we just need to
fix the regression in the !bs->drv case.

I feel a bit queasy about detecting -ENOMEDIUM, because it's not obvious
to me that bdrv_commit() can only return that when !bs->drv.