Re: [PATCH v2 02/11] softmmu/dirtylimit: Add parameter check for hmp "set_vcpu_dirty_limit"

[Markus Armbruster]

huang...@chinatelecom.cn writes:

> From: Hyman Huang(黄勇) <huang...@chinatelecom.cn>
>
> dirty_rate paraemter of hmp command "set_vcpu_dirty_limit" is invalid
> if less than 0, so add parameter check for it.
>
> Signed-off-by: Hyman Huang(黄勇) <huang...@chinatelecom.cn>
> ---
>  softmmu/dirtylimit.c | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/softmmu/dirtylimit.c b/softmmu/dirtylimit.c
> index 940d238..c42eddd 100644
> --- a/softmmu/dirtylimit.c
> +++ b/softmmu/dirtylimit.c
> @@ -515,6 +515,11 @@ void hmp_set_vcpu_dirty_limit(Monitor *mon, const QDict 
> *qdict)
   void hmp_set_vcpu_dirty_limit(Monitor *mon, const QDict *qdict)
   {
       int64_t dirty_rate = qdict_get_int(qdict, "dirty_rate");
>      int64_t cpu_index = qdict_get_try_int(qdict, "cpu_index", -1);
>      Error *err = NULL;
>  
> +    if (dirty_rate < 0) {
> +        monitor_printf(mon, "invalid dirty page limit %ld\n", dirty_rate);

Here, you use monitor_printf() to report an error, and ...

> +        return;
> +    }
> +
>      qmp_set_vcpu_dirty_limit(!!(cpu_index != -1), cpu_index, dirty_rate, 
> &err);
>      if (err) {
>          hmp_handle_error(mon, err);

... here you use hmp_handle_error().  Suggest to use the latter
consistently.

           return;
       }

       monitor_printf(mon, "[Please use 'info vcpu_dirty_limit' to query "
                      "dirty limit for virtual CPU]\n");

This prints unsolicited help how to read the setting every time you
change it.  We don't that.  Please delete.

   }

Together, the function could look like this:

void hmp_set_vcpu_dirty_limit(Monitor *mon, const QDict *qdict)
{
    int64_t dirty_rate = qdict_get_int(qdict, "dirty_rate");
    int64_t cpu_index = qdict_get_try_int(qdict, "cpu_index", -1);
    Error *err = NULL;

    if (dirty_rate < 0) {
        error_setg(&err, "invalid dirty page limit %ld", dirty_rate);
        goto out;
    }

    qmp_set_vcpu_dirty_limit(!!(cpu_index != -1), cpu_index, dirty_rate, &err);

out:
    hmp_handle_error(mon, err);
}