On Mon, Jul 19, 2010 at 11:22 AM, Kevin Wolf <kw...@redhat.com> wrote:
>>
>> - /* Verify if there is a device that doesn't support snapshots and is
>> writable */
>> + bs_vm_state = bdrv_snapshots();
>> + if (!bs_vm_state) {
>> + error_report("No block device supports snapshots");
>> + return -EINVAL;
>
> -ENOTSUP?
It was -EINVAL before, just kept it. But -ENOTSUP make more sense.
>> + /* Don't even try to load empty VM states */
>> + ret = bdrv_snapshot_find(bs_vm_state, &sn, name);
>> + if ((ret >= 0) && (sn.vm_state_size == 0)) {
>> + return -EINVAL;
>> + }
>
> You can probably stop here already if ret < 0:
>
> ret = ...
> if (ret < 0) {
> return ret;
> } else if (sn.vm_state_size == 0) {
> return -EINVAL;
> }
>
Better indeed.
>>
>> @@ -1821,64 +1834,46 @@ int load_vmstate(const char *name)
>> error_report("Device '%s' is writable but does not support
>> snapshots.",
>> bdrv_get_device_name(bs));
>> return -ENOTSUP;
>> + } else {
>
> The then branch has a return, so you don't need the else here and can
> have the following code nested one level less.
>
Ack.
I will send v2 shortly.
Thanks,
Miguel
