On Sat, May 29, 2010 at 3:06 AM, Markus Armbruster <arm...@redhat.com> 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
