[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH 3/5] qemu_init_vcpu: add a new Error paramater t

From: Fei Li
Subject: Re: [Qemu-devel] [PATCH 3/5] qemu_init_vcpu: add a new Error paramater to propagate
Date: Thu, 6 Sep 2018 14:52:25 +0800
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1

On 09/06/2018 01:15 AM, Eric Blake wrote:
Adding Markus, as the maintainer of Error APIs, to make sure he sees this thread.
Thanks. :)

On 09/04/2018 06:22 AM, Daniel P. Berrangé wrote:
On Tue, Sep 04, 2018 at 07:08:20PM +0800, Fei Li wrote:

In the subject line: s/paramater/parameter/
Thanks for pointing this out! :)

The caller of qemu_init_vcpu() already passed the **errp to handle
errors. In view of this, add a new Error parameter to the following
call trace to propagate the error and let the final caller check it.

Signed-off-by: Fei Li <address@hidden>

-void qemu_init_vcpu(CPUState *cpu)
+void qemu_init_vcpu(CPUState *cpu, Error **errp)
      cpu->nr_cores = smp_cores;
      cpu->nr_threads = smp_threads;
      cpu->stopped = true;
+    Error *local_err = NULL;
        if (!cpu->as) {
          /* If the target cpu hasn't set up any address spaces itself,
@@ -2046,17 +2047,22 @@ void qemu_init_vcpu(CPUState *cpu)
        if (kvm_enabled()) {
-        qemu_kvm_start_vcpu(cpu);
+        qemu_kvm_start_vcpu(cpu, &local_err);
      } else if (hax_enabled()) {
-        qemu_hax_start_vcpu(cpu);
+        qemu_hax_start_vcpu(cpu, &local_err);
      } else if (hvf_enabled()) {
-        qemu_hvf_start_vcpu(cpu);
+        qemu_hvf_start_vcpu(cpu, &local_err);
      } else if (tcg_enabled()) {
-        qemu_tcg_init_vcpu(cpu);
+        qemu_tcg_init_vcpu(cpu, &local_err);
      } else if (whpx_enabled()) {
-        qemu_whpx_start_vcpu(cpu);
+        qemu_whpx_start_vcpu(cpu, &local_err);
      } else {
-        qemu_dummy_start_vcpu(cpu);
+        qemu_dummy_start_vcpu(cpu, &local_err);
+    }
+    if (local_err) {
+        error_propagate(errp, local_err);
+        return;

I'd be inclined to make this method return a boolean, so....

diff --git a/target/alpha/cpu.c b/target/alpha/cpu.c
index b08078e7fc..5b0b4892f2 100644
--- a/target/alpha/cpu.c
+++ b/target/alpha/cpu.c
@@ -66,7 +66,11 @@ static void alpha_cpu_realizefn(DeviceState *dev, Error **errp)
  -    qemu_init_vcpu(cs);
+    qemu_init_vcpu(cs, &local_err);
+    if (local_err) {
+        error_propagate(errp, local_err);
+        return;
+    }

...this can be simplified to get rid of the local error object

   if (!qemu_init_vcpu(cs, errp)) {

Returning a bool introduces an interesting semantic question on whether 0 is success or failure (-1/0 vs. false/true means you have to pay attention to return type to decide between !func() or func()<0 when using the return value to learn if errp was set).  But Markus has expressed some thoughts on the matter before:


and favored a bool return if only for consistency with glib and to avoid abuse of trying to overload the returned int when using errp is better.

Thanks for providing these instructive discussions, I read the link, and yes, I agree with "return a bool: false/true inside the callee". Just as the following:

    if (!foo(arg, errp)) { // assuming foo() returns a bool: false/true
        handle the error...

As this is "Nicely concise." :)
I will prepare the next version according to all the comments.

Have a nice day, thanks all

reply via email to

[Prev in Thread] Current Thread [Next in Thread]