Re: [Qemu-devel] [RFC PATCH v4 04/25] sysemu: system functions for replay

["Alex Bennée" <alex.bennee@linaro.org>]

Pavel Dovgalyuk <Pavel.Dovgaluk@ispras.ru> writes:

> This patch removes "static" specifier from several qemu function to make
> them visible to the replay module. It also invents several system functions
> that will be used by replay.
>
> Signed-off-by: Pavel Dovgalyuk <pavel.dovgaluk@ispras.ru>
> ---
<snip>
>  
>  void do_savevm(Monitor *mon, const QDict *qdict);
>  int load_vmstate(const char *name);
> +int save_vmstate(Monitor *mon, const char *name);
>  void do_delvm(Monitor *mon, const QDict *qdict);
>  void do_info_snapshots(Monitor *mon, const QDict *qdict);
<snip>
>  
> -void do_savevm(Monitor *mon, const QDict *qdict)
> +int save_vmstate(Monitor *mon, const char *name)
>  {
>      BlockDriverState *bs, *bs1;
>      QEMUSnapshotInfo sn1, *sn = &sn1, old_sn1, *old_sn = &old_sn1;
> @@ -1049,7 +1049,7 @@ void do_savevm(Monitor *mon, const QDict *qdict)
>      uint64_t vm_state_size;
>      qemu_timeval tv;
>      struct tm tm;
> -    const char *name = qdict_get_try_str(qdict, "name");
> +    int success = 0;
>  
>      /* Verify if there is a device that doesn't support snapshots and is writable */
>      bs = NULL;
> @@ -1062,14 +1062,19 @@ void do_savevm(Monitor *mon, const QDict *qdict)
>          if (!bdrv_can_snapshot(bs)) {
>              monitor_printf(mon, "Device '%s' is writable but does not support snapshots.\n",
>                                 bdrv_get_device_name(bs));
> -            return;
> +            return success;
>          }
>      }
>  
>      bs = find_vmstate_bs();
>      if (!bs) {
>          monitor_printf(mon, "No block device can accept snapshots\n");
> -        return;
> +        if (replay_mode != REPLAY_MODE_NONE) {
> +            fprintf(stderr,
> +                    "At least one hdd should be attached to QEMU for replay\n");
> +            exit(1);

Perhaps we should be doing a proper error_report here?

> +        }
> +        return success;
>      }
>  
>      saved_vm_running = runstate_is_running();
> @@ -1118,6 +1123,7 @@ void do_savevm(Monitor *mon, const QDict *qdict)
>  
>      /* create the snapshots */
>  
> +    success = 1;
>      bs1 = NULL;
>      while ((bs1 = bdrv_next(bs1))) {
>          if (bdrv_can_snapshot(bs1)) {
> @@ -1127,6 +1133,7 @@ void do_savevm(Monitor *mon, const QDict *qdict)
>              if (ret < 0) {
>                  monitor_printf(mon, "Error while creating snapshot on '%s'\n",
>                                 bdrv_get_device_name(bs1));
> +                success = 0;
>              }
>          }
>      }
> @@ -1135,6 +1142,14 @@ void do_savevm(Monitor *mon, const QDict *qdict)
>      if (saved_vm_running) {
>          vm_start();
>      }
> +
> +    return success;
> +}
> +
> +void do_savevm(Monitor *mon, const QDict *qdict)
> +{
> +    const char *name = qdict_get_try_str(qdict, "name");
> +    save_vmstate(mon, name);
>  }

You've re-factored do_savevm() and added a success/fail parameter which
isn't used by the caller. A bit of documentation on what success means
in this context would be useful for the function.

My personal preference is for success/fail returns is to use unambiguous
bool types but we don't mandate that. 

-- 
Alex Bennée