Re: [Qemu-devel] [PATCH 1/3] cleanup: bdrv_snaphost_find() returns zero or -ENOENT

[Markus Armbruster]

Miguel Di Ciurcio Filho <address@hidden> writes:

> The bdrv_snaphost_find() returns zero in case it finds an snapshot or -ENOENT 
> in
> case it doesn't.
>
> Checking returning values as >= zero doesn't make sense.

Debatable.  "RETVAL < 0" is an idiomatic check for error.  "RETVAL >= 0"
is merely its negation.

Anyway, I do prefer code like this

    ret = do_something();
    if (ret < 0) {
        handle error...
    }
    do more...

over

    ret = do_something();
    if (ret >= 0) {
        do more...
    }

>
> Signed-off-by: Miguel Di Ciurcio Filho <address@hidden>
> ---
>  savevm.c |    7 ++++---
>  1 files changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/savevm.c b/savevm.c
> index 7a1de3c..6c6adb0 100644
> --- a/savevm.c
> +++ b/savevm.c
> @@ -1768,7 +1768,7 @@ static int del_existing_snapshots(Monitor *mon, const 
> char *name)
>      bs = NULL;
>      while ((bs = bdrv_next(bs))) {
>          if (bdrv_can_snapshot(bs) &&
> -            bdrv_snapshot_find(bs, snapshot, name) >= 0)
> +            bdrv_snapshot_find(bs, snapshot, name) == 0)
>          {
>              ret = bdrv_snapshot_delete(bs, name);
>              if (ret < 0) {
> @@ -1948,8 +1948,9 @@ int load_vmstate(const char *name)
>  
>      /* Don't even try to load empty VM states */
>      ret = bdrv_snapshot_find(bs, &sn, name);
> -    if ((ret >= 0) && (sn.vm_state_size == 0))
> -        return -EINVAL;
> +    if ((ret == 0) && (sn.vm_state_size == 0)) {
> +            return -EINVAL;
> +    }
>  
>      /* restore the VM state */
>      f = qemu_fopen_bdrv(bs, 0);

I wonder what happens if bdrv_snapshot_find() fails.  Why is it safe to
ignore that and continue?


do_savevm() has another one:

        ret = bdrv_snapshot_find(bs, old_sn, name);
        if (ret >= 0) {
            pstrcpy(sn->name, sizeof(sn->name), old_sn->name);
            pstrcpy(sn->id_str, sizeof(sn->id_str), old_sn->id_str);
        } else {
            pstrcpy(sn->name, sizeof(sn->name), name);
        }