[Qemu-devel] Re: [PATCH] savevm: Really verify if a drive supports snapshots

[Miguel Di Ciurcio Filho]

On Sat, May 29, 2010 at 3:06 AM, Markus Armbruster <address@hidden> wrote:
>
>> I seem to remember that we came to the conclusion that
>> bdrv_has_snapshot() isn't needed at all and should be dropped. Any user
>> should be using bdrv_can_snapshot() instead as this is what they really
>> want.
>
> Our reasoning adapted to the patched code goes like this.  The remaining
> uses of bdrv_has_snapshot() are of the form:
>
>    if (bdrv_has_snapshot(bs)
>       some_snapshot_op()
>
> where some_snapshot_op() fails when there are no snapshots, and the code
> can recognize that failure as "sorry, no snapshots" (that's what it does
> before your patch, when bdrv_has_snapshot() is really no more than a
> necessary, but not sufficient condition for "has snapshots").
>
> Therefore, we don't have to check for actual existence of snapshots with
> bdrv_has_snapshot().  bdrv_can_snapshot() suffices.

I had this feeling too, but I was conservative and kept
bdrv_has_snapshot(). I will remove it and replace by
bdrv_can_snapshot() where appropriate.

>>> +int bdrv_can_snapshot(BlockDriverState *bs)
>>>  {
>>>      BlockDriver *drv = bs->drv;
>>> -    if (!drv)
>>> +    if (!drv) {
>>> +        return -ENOMEDIUM;
>>> +    }
>>> +
>>> +    if (!drv->bdrv_snapshot_create || bdrv_is_removable(bs) ||
>>> +        bdrv_is_read_only(bs)) {
>>> +        return -ENOTSUP;
>>> +    }
>>> +
>>> +    return 1;
>>> +}
>>
>> Returning either 1 or -errno is a strange interface. I'm not sure which
>
> Agree.
>
>> of 1/0 or 0/-errno is better in this case, but I'd suggest to take one
>> of these.
>
> Does any existing caller care for the error codes?  If not, just go with
> 1/0.

When bdrv_can_snapshot() is called the specific reason for not
supporting snapshots does not matter. So, 1/0 is better. I will fix it
and resend.

Regards,

Miguel