diff mbox series

[QEMU,v5,1/8] softmmu/dirtylimit: Add parameter check for hmp "set_vcpu_dirty_limit"

Message ID 168618975839.6361.17407633874747688653-1@git.sr.ht
State New
Headers show
Series migration: introduce dirtylimit capability | expand

Commit Message

~hyman Nov. 18, 2022, 2:08 a.m. UTC 
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(-)

Comments

Juan Quintela June 13, 2023, 4:20 p.m. UTC | #1 
~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 June 13, 2023, 4:37 p.m. UTC | #2 
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.
diff mbox series

Patch

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)