From: Hyman Huang(黄勇) <yong.huang@smartx.com>

dirty_rate paraemter of hmp command "set_vcpu_dirty_limit" is invalid
if less than 0, so add parameter check for it.

Note that this patch also delete the unsolicited help message and
clean up the code.

Signed-off-by: Hyman Huang(黄勇) <yong.huang@smartx.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Peter Xu <peterx@redhat.com>
---
 softmmu/dirtylimit.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/softmmu/dirtylimit.c b/softmmu/dirtylimit.c
index 015a9038d1..5c12d26d49 100644
--- a/softmmu/dirtylimit.c
+++ b/softmmu/dirtylimit.c
@@ -515,14 +515,15 @@ void hmp_set_vcpu_dirty_limit(Monitor *mon, const QDict *qdict)
     int64_t cpu_index = qdict_get_try_int(qdict, "cpu_index", -1);
     Error *err = NULL;
 
-    qmp_set_vcpu_dirty_limit(!!(cpu_index != -1), cpu_index, dirty_rate, &err);
-    if (err) {
-        hmp_handle_error(mon, err);
-        return;
+    if (dirty_rate < 0) {
+        error_setg(&err, "invalid dirty page limit %ld", dirty_rate);
+        goto out;
     }
 
-    monitor_printf(mon, "[Please use 'info vcpu_dirty_limit' to query "
-                   "dirty limit for virtual CPU]\n");
+    qmp_set_vcpu_dirty_limit(!!(cpu_index != -1), cpu_index, dirty_rate, &err);
+
+out:
+    hmp_handle_error(mon, err);
 }
 
 static struct DirtyLimitInfo *dirtylimit_query_vcpu(int cpu_index)
-- 
2.38.5

~hyman <hyman@git.sr.ht> wrote:
> From: Hyman Huang(黄勇) <yong.huang@smartx.com>
>
> dirty_rate paraemter of hmp command "set_vcpu_dirty_limit" is invalid
> if less than 0, so add parameter check for it.

why?

Next patch does it correctly:

+    if (params->has_x_vcpu_dirty_limit_period &&
+        (params->x_vcpu_dirty_limit_period < 1 ||
+         params->x_vcpu_dirty_limit_period > 1000)) {
+        error_setg(errp, QERR_INVALID_PARAMETER_VALUE,
+                   "x-vcpu-dirty-limit-period",
+                   "a value between 1 and 1000");
+        return false;
+    }
+
     return true;
 }

I hate to have to search several places to check for errors in values.
We get all errors in the functions that set the parameters.

Can you resend with just the monitor command removed?

Or there is any advantage of getting the error message from
qemu_set_vcpu_dirty_limit()?

Later, Juan.

Juan Quintela <quintela@redhat.com> wrote:
> ~hyman <hyman@git.sr.ht> wrote:
>> From: Hyman Huang(黄勇) <yong.huang@smartx.com>
>>
>> dirty_rate paraemter of hmp command "set_vcpu_dirty_limit" is invalid
>> if less than 0, so add parameter check for it.
>
> why?

And here I am, making a full of myself.

vcpu_dirty_limit and vcpu_dirty_limit_period are two different things.

So:

Reviewed-by: Juan Quintela <quintela@redhat.com>


> Next patch does it correctly:
>
> +    if (params->has_x_vcpu_dirty_limit_period &&
> +        (params->x_vcpu_dirty_limit_period < 1 ||
> +         params->x_vcpu_dirty_limit_period > 1000)) {
> +        error_setg(errp, QERR_INVALID_PARAMETER_VALUE,
> +                   "x-vcpu-dirty-limit-period",
> +                   "a value between 1 and 1000");
> +        return false;
> +    }
> +
>      return true;
>  }
>
> I hate to have to search several places to check for errors in values.
> We get all errors in the functions that set the parameters.
>
> Can you resend with just the monitor command removed?
>
> Or there is any advantage of getting the error message from
> qemu_set_vcpu_dirty_limit()?
>
> Later, Juan.