pc/spapr/s390x: machine hotplug handler cleanups

Let's introduce and use local error variables in the hotplug handler functions.

Signed-off-by David Hildenbrand <[email protected]>

---
 hw/i386/pc.c | 32 ++++++++++++++++++++++----------
 1 file changed, 22 insertions(+), 10 deletions(-)

hw/i386/pc.c +22 -10

index f3befe6721..8075c6af15 100644
@@ -2006,45 +2006,57 @@ static void pc_cpu_pre_plug(HotplugHandler *hotplug_dev,
 static void pc_machine_device_pre_plug_cb(HotplugHandler *hotplug_dev,
                                           DeviceState *dev, Error **errp)
 {
+    Error *local_err = NULL;
+
     if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
-        pc_cpu_pre_plug(hotplug_dev, dev, errp);
+        pc_cpu_pre_plug(hotplug_dev, dev, &local_err);
     }
+    error_propagate(errp, local_err);
 }
 
 static void pc_machine_device_plug_cb(HotplugHandler *hotplug_dev,
                                       DeviceState *dev, Error **errp)
 {
+    Error *local_err = NULL;
+
     if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
-        pc_dimm_plug(hotplug_dev, dev, errp);
+        pc_dimm_plug(hotplug_dev, dev, &local_err);
     } else if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
-        pc_cpu_plug(hotplug_dev, dev, errp);
+        pc_cpu_plug(hotplug_dev, dev, &local_err);
     }
+    error_propagate(errp, local_err);
 }
 
 static void pc_machine_device_unplug_request_cb(HotplugHandler *hotplug_dev,
                                                 DeviceState *dev, Error **errp)
 {
+    Error *local_err = NULL;
+
     if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
-        pc_dimm_unplug_request(hotplug_dev, dev, errp);
+        pc_dimm_unplug_request(hotplug_dev, dev, &local_err);
     } else if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
-        pc_cpu_unplug_request_cb(hotplug_dev, dev, errp);
+        pc_cpu_unplug_request_cb(hotplug_dev, dev, &local_err);
     } else {
-        error_setg(errp, "acpi: device unplug request for not supported device"
-                   " type: %s", object_get_typename(OBJECT(dev)));
+        error_setg(&local_err, "acpi: device unplug request for not supported"
+                   " device type: %s", object_get_typename(OBJECT(dev)));
     }
+    error_propagate(errp, local_err);
 }
 
 static void pc_machine_device_unplug_cb(HotplugHandler *hotplug_dev,
                                         DeviceState *dev, Error **errp)
 {
+    Error *local_err = NULL;
+
     if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
-        pc_dimm_unplug(hotplug_dev, dev, errp);
+        pc_dimm_unplug(hotplug_dev, dev, &local_err);
     } else if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
-        pc_cpu_unplug_cb(hotplug_dev, dev, errp);
+        pc_cpu_unplug_cb(hotplug_dev, dev, &local_err);
     } else {
-        error_setg(errp, "acpi: device unplug for not supported device"
+        error_setg(&local_err, "acpi: device unplug for not supported device"
                    " type: %s", object_get_typename(OBJECT(dev)));
     }
+    error_propagate(errp, local_err);
 }
 
 static HotplugHandler *pc_get_hotpug_handler(MachineState *machine,

Quoted 2 lines · David Hildenbrand

Let's introduce and use local error variables in the hotplug handler functions.

Why? You don't check local_err in the functions, so I fail to see why this is needed? If you need this in a later patch, I think this should simply be part of that later patch instead.

Thomas

Quoted 78 lines

Signed-off-by David Hildenbrand <[email protected]>

---
 hw/i386/pc.c | 32 ++++++++++++++++++++++----------
 1 file changed, 22 insertions(+), 10 deletions(-)

hw/i386/pc.c +22 -10

index f3befe6721..8075c6af15 100644
@@ -2006,45 +2006,57 @@ static void pc_cpu_pre_plug(HotplugHandler *hotplug_dev,
 static void pc_machine_device_pre_plug_cb(HotplugHandler *hotplug_dev,
                                           DeviceState *dev, Error **errp)
 {
+    Error *local_err = NULL;
+
     if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
-        pc_cpu_pre_plug(hotplug_dev, dev, errp);
+        pc_cpu_pre_plug(hotplug_dev, dev, &local_err);
     }
+    error_propagate(errp, local_err);
 }
 
 static void pc_machine_device_plug_cb(HotplugHandler *hotplug_dev,
                                       DeviceState *dev, Error **errp)
 {
+    Error *local_err = NULL;
+
     if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
-        pc_dimm_plug(hotplug_dev, dev, errp);
+        pc_dimm_plug(hotplug_dev, dev, &local_err);
     } else if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
-        pc_cpu_plug(hotplug_dev, dev, errp);
+        pc_cpu_plug(hotplug_dev, dev, &local_err);
     }
+    error_propagate(errp, local_err);
 }
 
 static void pc_machine_device_unplug_request_cb(HotplugHandler *hotplug_dev,
                                                 DeviceState *dev, Error **errp)
 {
+    Error *local_err = NULL;
+
     if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
-        pc_dimm_unplug_request(hotplug_dev, dev, errp);
+        pc_dimm_unplug_request(hotplug_dev, dev, &local_err);
     } else if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
-        pc_cpu_unplug_request_cb(hotplug_dev, dev, errp);
+        pc_cpu_unplug_request_cb(hotplug_dev, dev, &local_err);
     } else {
-        error_setg(errp, "acpi: device unplug request for not supported device"
-                   " type: %s", object_get_typename(OBJECT(dev)));
+        error_setg(&local_err, "acpi: device unplug request for not supported"
+                   " device type: %s", object_get_typename(OBJECT(dev)));
     }
+    error_propagate(errp, local_err);
 }
 
 static void pc_machine_device_unplug_cb(HotplugHandler *hotplug_dev,
                                         DeviceState *dev, Error **errp)
 {
+    Error *local_err = NULL;
+
     if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
-        pc_dimm_unplug(hotplug_dev, dev, errp);
+        pc_dimm_unplug(hotplug_dev, dev, &local_err);
     } else if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
-        pc_cpu_unplug_cb(hotplug_dev, dev, errp);
+        pc_cpu_unplug_cb(hotplug_dev, dev, &local_err);
     } else {
-        error_setg(errp, "acpi: device unplug for not supported device"
+        error_setg(&local_err, "acpi: device unplug for not supported device"
                    " type: %s", object_get_typename(OBJECT(dev)));
     }
+    error_propagate(errp, local_err);
 }
 
 static HotplugHandler *pc_get_hotpug_handler(MachineState *machine,

The node property can always be queried and the value has already been verified in pc_dimm_realize().

Signed-off-by David Hildenbrand <[email protected]>

---
 hw/ppc/spapr.c | 9 +--------
 1 file changed, 1 insertion(+), 8 deletions(-)

hw/ppc/spapr.c +1 -8

index 2375cbee12..d038f3243e 100644
@@ -3578,14 +3578,7 @@ static void spapr_machine_device_plug(HotplugHandler *hotplug_dev,
             error_setg(errp, "Memory hotplug not supported for this machine");
             return;
         }
-        node = object_property_get_uint(OBJECT(dev), PC_DIMM_NODE_PROP, errp);
-        if (*errp) {
-            return;
-        }
-        if (node < 0 || node >= MAX_NODES) {
-            error_setg(errp, "Invaild node %d", node);
-            return;
-        }
+        node = object_property_get_uint(OBJECT(dev), PC_DIMM_NODE_PROP, NULL);
 
         spapr_memory_plug(hotplug_dev, dev, node, errp);
     } else if (object_dynamic_cast(OBJECT(dev), TYPE_SPAPR_CPU_CORE)) {

The node property can always be queried and the value has already been verified in pc_dimm_realize().

Signed-off-by David Hildenbrand <[email protected]>

---
 hw/ppc/spapr.c | 9 +--------
 1 file changed, 1 insertion(+), 8 deletions(-)

hw/ppc/spapr.c +0 -0

index 2375cbee12..d038f3243e 100644

hw/ppc/spapr.c +0 -0

         spapr_memory_plug(hotplug_dev, dev, node, errp);
     } else if (object_dynamic_cast(OBJECT(dev), TYPE_SPAPR_CPU_CORE)) {

Quoted 13 lines · On Thu, 7 Jun 2018 18:52:12 +0200 David Hildenbrand <[email protected]> wrote:

The node property can always be queried and the value has already been verified in pc_dimm_realize().

Signed-off-by David Hildenbrand <[email protected]>

---
 hw/ppc/spapr.c | 9 +--------
 1 file changed, 1 insertion(+), 8 deletions(-)

hw/ppc/spapr.c +0 -0

index 2375cbee12..d038f3243e 100644

hw/ppc/spapr.c static void spapr_machine_device_plug(HotplugHandler *hotplug_dev, · line 3582

Quoted 5 lines

hw/ppc/spapr.c +0 -2

@@ -3578,14 +3578,7 @@ static void spapr_machine_device_plug(HotplugHandler *hotplug_dev,
             error_setg(errp, "Memory hotplug not supported for this machine");
             return;
         }
-        node = object_property_get_uint(OBJECT(dev), PC_DIMM_NODE_PROP, errp);
-        if (*errp) {

Good riddance :)

hw/ppc/spapr.c static void spapr_machine_device_plug(HotplugHandler *hotplug_dev, · line 3581

Quoted 7 lines

hw/ppc/spapr.c +1 -6

-            return;
-        }
-        if (node < 0 || node >= MAX_NODES) {
-            error_setg(errp, "Invaild node %d", node);
-            return;
-        }
+        node = object_property_get_uint(OBJECT(dev), PC_DIMM_NODE_PROP, NULL);

Maybe pass &error_abort ?

Quoted 4 lines

hw/ppc/spapr.c +0 -0

         spapr_memory_plug(hotplug_dev, dev, node, errp);
     } else if (object_dynamic_cast(OBJECT(dev), TYPE_SPAPR_CPU_CORE)) {

Quoted 32 lines · Greg Kurz

Quoted 13 lines · On Thu, 7 Jun 2018 18:52:12 +0200 David Hildenbrand <[email protected]> wrote:

The node property can always be queried and the value has already been verified in pc_dimm_realize().

Signed-off-by David Hildenbrand <[email protected]>

---
 hw/ppc/spapr.c | 9 +--------
 1 file changed, 1 insertion(+), 8 deletions(-)

hw/ppc/spapr.c +0 -0

index 2375cbee12..d038f3243e 100644

hw/ppc/spapr.c static void spapr_machine_device_plug(HotplugHandler *hotplug_dev, · line 3582

Quoted 5 lines

hw/ppc/spapr.c +0 -2

@@ -3578,14 +3578,7 @@ static void spapr_machine_device_plug(HotplugHandler *hotplug_dev,
             error_setg(errp, "Memory hotplug not supported for this machine");
             return;
         }
-        node = object_property_get_uint(OBJECT(dev), PC_DIMM_NODE_PROP, errp);
-        if (*errp) {  

Good riddance :)

Quoted 7 lines

hw/ppc/spapr.c +1 -6

-            return;
-        }
-        if (node < 0 || node >= MAX_NODES) {
-            error_setg(errp, "Invaild node %d", node);
-            return;
-        }
+        node = object_property_get_uint(OBJECT(dev), PC_DIMM_NODE_PROP, NULL);  

Quoted 1 line

Maybe pass &error_abort ?

I'm using the same access scheme as in hw/acpi/memory_hotplug.c

("error ignored" vs. "error leads to an abort") - but this will actually never fail. But I can use error_abort here, does not matter.

Thanks!

Quoted 5 lines

Quoted 4 lines

hw/ppc/spapr.c +0 -0

         spapr_memory_plug(hotplug_dev, dev, node, errp);
     } else if (object_dynamic_cast(OBJECT(dev), TYPE_SPAPR_CPU_CORE)) {  

Quoted 35 lines · On Fri, 8 Jun 2018 09:42:48 +0200 David Hildenbrand <[email protected]> wrote:

Quoted 33 lines · Greg Kurz

Quoted 13 lines · On Thu, 7 Jun 2018 18:52:12 +0200 David Hildenbrand <[email protected]> wrote:

The node property can always be queried and the value has already been verified in pc_dimm_realize().

Signed-off-by David Hildenbrand <[email protected]>

---
 hw/ppc/spapr.c | 9 +--------
 1 file changed, 1 insertion(+), 8 deletions(-)

hw/ppc/spapr.c +0 -0

index 2375cbee12..d038f3243e 100644

hw/ppc/spapr.c static void spapr_machine_device_plug(HotplugHandler *hotplug_dev, · line 3582

Quoted 5 lines

hw/ppc/spapr.c +0 -2

@@ -3578,14 +3578,7 @@ static void spapr_machine_device_plug(HotplugHandler *hotplug_dev,
             error_setg(errp, "Memory hotplug not supported for this machine");
             return;
         }
-        node = object_property_get_uint(OBJECT(dev), PC_DIMM_NODE_PROP, errp);
-        if (*errp) {  

Good riddance :)

hw/ppc/spapr.c static void spapr_machine_device_plug(HotplugHandler *hotplug_dev, · line 3581

Quoted 7 lines

hw/ppc/spapr.c +1 -6

-            return;
-        }
-        if (node < 0 || node >= MAX_NODES) {
-            error_setg(errp, "Invaild node %d", node);
-            return;
-        }
+        node = object_property_get_uint(OBJECT(dev), PC_DIMM_NODE_PROP, NULL);  

Maybe pass &error_abort ?

Quoted 5 lines

I'm using the same access scheme as in hw/acpi/memory_hotplug.c

("error ignored" vs. "error leads to an abort") - but this will actually never fail. But I can use error_abort here, does not matter.

Heh, /me paranoid but this is David's call and he acked that already so I guess it's okay.

Reviewed-by Greg Kurz <[email protected]>

Quoted 9 lines

Thanks!

Quoted 5 lines

Quoted 4 lines

hw/ppc/spapr.c +0 -0

         spapr_memory_plug(hotplug_dev, dev, node, errp);
     } else if (object_dynamic_cast(OBJECT(dev), TYPE_SPAPR_CPU_CORE)) {  

Quoted 28 lines · Greg Kurz

Quoted 25 lines · On Fri, 8 Jun 2018 09:42:48 +0200 David Hildenbrand <[email protected]> wrote:

Quoted 24 lines · Greg Kurz

Quoted 13 lines · On Thu, 7 Jun 2018 18:52:12 +0200 David Hildenbrand <[email protected]> wrote:

The node property can always be queried and the value has already been verified in pc_dimm_realize().

Signed-off-by David Hildenbrand <[email protected]>

---
 hw/ppc/spapr.c | 9 +--------
 1 file changed, 1 insertion(+), 8 deletions(-)

hw/ppc/spapr.c +0 -0

index 2375cbee12..d038f3243e 100644

hw/ppc/spapr.c static void spapr_machine_device_plug(HotplugHandler *hotplug_dev, · line 3582

Quoted 5 lines

hw/ppc/spapr.c +0 -2

@@ -3578,14 +3578,7 @@ static void spapr_machine_device_plug(HotplugHandler *hotplug_dev,
             error_setg(errp, "Memory hotplug not supported for this machine");
             return;
         }
-        node = object_property_get_uint(OBJECT(dev), PC_DIMM_NODE_PROP, errp);
-        if (*errp) {  

Good riddance :)

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

hw/ppc/spapr.c static void spapr_machine_device_plug(HotplugHandler *hotplug_dev, · line 3588

Quoted 6 lines

hw/ppc/spapr.c +0 -6

-            return;
-        }
-        if (node < 0 || node >= MAX_NODES) {
-            error_setg(errp, "Invaild node %d", node);
-            return;
-        }

Quoted 6 lines

Quoted 6 lines

Quoted 3 lines

hw/ppc/spapr.c static void spapr_machine_device_plug(HotplugHandler *hotplug_dev, · line 3581

Quoted 1 line

hw/ppc/spapr.c +1 -0

+        node = object_property_get_uint(OBJECT(dev), PC_DIMM_NODE_PROP, NULL);  

Maybe pass &error_abort ?

I'm using the same access scheme as in hw/acpi/memory_hotplug.c

Quoted 6 lines

Quoted 3 lines

("error ignored" vs. "error leads to an abort") - but this will actually never fail. But I can use error_abort here, does not matter.

Heh, /me paranoid but this is David's call and he acked that already so I guess it's okay.

NULL makes it fit into a single line :)

Quoted 29 lines · David Hildenbrand

Quoted 28 lines · Greg Kurz

Quoted 25 lines · On Fri, 8 Jun 2018 09:42:48 +0200 David Hildenbrand <[email protected]> wrote:

Quoted 24 lines · Greg Kurz

Quoted 13 lines · On Thu, 7 Jun 2018 18:52:12 +0200 David Hildenbrand <[email protected]> wrote:

The node property can always be queried and the value has already been verified in pc_dimm_realize().

Signed-off-by David Hildenbrand <[email protected]>

---
 hw/ppc/spapr.c | 9 +--------
 1 file changed, 1 insertion(+), 8 deletions(-)

hw/ppc/spapr.c +0 -0

index 2375cbee12..d038f3243e 100644

hw/ppc/spapr.c static void spapr_machine_device_plug(HotplugHandler *hotplug_dev, · line 3582

Quoted 5 lines

hw/ppc/spapr.c +0 -2

@@ -3578,14 +3578,7 @@ static void spapr_machine_device_plug(HotplugHandler *hotplug_dev,
             error_setg(errp, "Memory hotplug not supported for this machine");
             return;
         }
-        node = object_property_get_uint(OBJECT(dev), PC_DIMM_NODE_PROP, errp);
-        if (*errp) {    

Good riddance :)

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

hw/ppc/spapr.c static void spapr_machine_device_plug(HotplugHandler *hotplug_dev, · line 3588

Quoted 6 lines

hw/ppc/spapr.c +0 -6

-            return;
-        }
-        if (node < 0 || node >= MAX_NODES) {
-            error_setg(errp, "Invaild node %d", node);
-            return;
-        }  

Maybe turn that into an assert() instead? ... just for the paranoids ;-)

Quoted 10 lines

Quoted 10 lines

Quoted 9 lines

Quoted 3 lines

hw/ppc/spapr.c static void spapr_machine_device_plug(HotplugHandler *hotplug_dev, · line 3581

Quoted 1 line

hw/ppc/spapr.c +1 -0

+        node = object_property_get_uint(OBJECT(dev), PC_DIMM_NODE_PROP, NULL);    

Maybe pass &error_abort ?

I'm using the same access scheme as in hw/acpi/memory_hotplug.c

("error ignored" vs. "error leads to an abort") - but this will actually never fail. But I can use error_abort here, does not matter.

Quoted 4 lines

Quoted 2 lines

Heh, /me paranoid but this is David's call and he acked that already so I guess it's okay.

NULL makes it fit into a single line :)

+1 for error_abort, even if it takes another line.

Thomas

Quoted 52 lines · On Fri, 8 Jun 2018 10:07:31 +0200 Thomas Huth <[email protected]> wrote:

Quoted 29 lines · David Hildenbrand

Quoted 28 lines · Greg Kurz

Quoted 25 lines · On Fri, 8 Jun 2018 09:42:48 +0200 David Hildenbrand <[email protected]> wrote:

Quoted 24 lines · Greg Kurz

Quoted 13 lines · On Thu, 7 Jun 2018 18:52:12 +0200 David Hildenbrand <[email protected]> wrote:

The node property can always be queried and the value has already been verified in pc_dimm_realize().

Signed-off-by David Hildenbrand <[email protected]>

---
 hw/ppc/spapr.c | 9 +--------
 1 file changed, 1 insertion(+), 8 deletions(-)

hw/ppc/spapr.c +0 -0

index 2375cbee12..d038f3243e 100644

hw/ppc/spapr.c static void spapr_machine_device_plug(HotplugHandler *hotplug_dev, · line 3582

Quoted 5 lines

hw/ppc/spapr.c +0 -2

@@ -3578,14 +3578,7 @@ static void spapr_machine_device_plug(HotplugHandler *hotplug_dev,
             error_setg(errp, "Memory hotplug not supported for this machine");
             return;
         }
-        node = object_property_get_uint(OBJECT(dev), PC_DIMM_NODE_PROP, errp);
-        if (*errp) {    

Good riddance :)

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

hw/ppc/spapr.c static void spapr_machine_device_plug(HotplugHandler *hotplug_dev, · line 3588

Quoted 6 lines

hw/ppc/spapr.c +0 -6

-            return;
-        }
-        if (node < 0 || node >= MAX_NODES) {
-            error_setg(errp, "Invaild node %d", node);
-            return;
-        }  

Maybe turn that into an assert() instead? ... just for the paranoids ;-)

Quoted 13 lines

Quoted 12 lines

Quoted 9 lines

Quoted 3 lines

hw/ppc/spapr.c static void spapr_machine_device_plug(HotplugHandler *hotplug_dev, · line 3581

Quoted 1 line

hw/ppc/spapr.c +1 -0

+        node = object_property_get_uint(OBJECT(dev), PC_DIMM_NODE_PROP, NULL);    

Maybe pass &error_abort ?

I'm using the same access scheme as in hw/acpi/memory_hotplug.c

("error ignored" vs. "error leads to an abort") - but this will actually never fail. But I can use error_abort here, does not matter.

Heh, /me paranoid but this is David's call and he acked that already so I guess it's okay.

Quoted 3 lines

Quoted 1 line

NULL makes it fit into a single line :)

+1 for error_abort, even if it takes another line.

+1 for error_abort
call shouldn't fail, but if does it won't be silently ignored and introduce undefined behavior.

Quoted 3 lines

Thomas

Quoted 57 lines · Igor Mammedov

Quoted 54 lines · On Fri, 8 Jun 2018 10:07:31 +0200 Thomas Huth <[email protected]> wrote:

Quoted 29 lines · David Hildenbrand

Quoted 28 lines · Greg Kurz

Quoted 25 lines · On Fri, 8 Jun 2018 09:42:48 +0200 David Hildenbrand <[email protected]> wrote:

Quoted 24 lines · Greg Kurz

Quoted 13 lines · On Thu, 7 Jun 2018 18:52:12 +0200 David Hildenbrand <[email protected]> wrote:

The node property can always be queried and the value has already been verified in pc_dimm_realize().

Signed-off-by David Hildenbrand <[email protected]>

---
 hw/ppc/spapr.c | 9 +--------
 1 file changed, 1 insertion(+), 8 deletions(-)

hw/ppc/spapr.c +0 -0

index 2375cbee12..d038f3243e 100644

hw/ppc/spapr.c static void spapr_machine_device_plug(HotplugHandler *hotplug_dev, · line 3582

Quoted 5 lines

hw/ppc/spapr.c +0 -2

@@ -3578,14 +3578,7 @@ static void spapr_machine_device_plug(HotplugHandler *hotplug_dev,
             error_setg(errp, "Memory hotplug not supported for this machine");
             return;
         }
-        node = object_property_get_uint(OBJECT(dev), PC_DIMM_NODE_PROP, errp);
-        if (*errp) {      

Good riddance :)

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

hw/ppc/spapr.c static void spapr_machine_device_plug(HotplugHandler *hotplug_dev, · line 3588

Quoted 6 lines

hw/ppc/spapr.c +0 -6

-            return;
-        }
-        if (node < 0 || node >= MAX_NODES) {
-            error_setg(errp, "Invaild node %d", node);
-            return;
-        }    

Maybe turn that into an assert() instead? ... just for the paranoids ;-)

Quoted 14 lines

Quoted 4 lines

Quoted 4 lines

Quoted 3 lines

hw/ppc/spapr.c static void spapr_machine_device_plug(HotplugHandler *hotplug_dev, · line 3581

Quoted 1 line

hw/ppc/spapr.c +1 -0

+        node = object_property_get_uint(OBJECT(dev), PC_DIMM_NODE_PROP, NULL);      

Maybe pass &error_abort ?

Quoted 8 lines

Quoted 5 lines

I'm using the same access scheme as in hw/acpi/memory_hotplug.c

("error ignored" vs. "error leads to an abort") - but this will actually never fail. But I can use error_abort here, does not matter.

Heh, /me paranoid but this is David's call and he acked that already so I guess it's okay.

NULL makes it fit into a single line :)

Quoted 4 lines

Quoted 1 line

+1 for error_abort, even if it takes another line.

+1 for error_abort
call shouldn't fail, but if does it won't be silently ignored and introduce undefined behavior.

Maybe we should fix the others that pass in NULL.

(no, not me :D - I'm already busy with your requested pre_plug handling)

Quoted 5 lines

Quoted 4 lines

Thomas

Quoted 9 lines

Quoted 5 lines

Quoted 2 lines

+1 for error_abort, even if it takes another line.

+1 for error_abort
call shouldn't fail, but if does it won't be silently ignored and introduce undefined behavior.

Maybe we should fix the others that pass in NULL.

(no, not me :D - I'm already busy with your requested pre_plug handling)

Add it to wiki page for bite sized tasks?

Quoted 5 lines

Quoted 5 lines

Quoted 4 lines

Quoted 1 line

+1 for error_abort, even if it takes another line.

+1 for error_abort
call shouldn't fail, but if does it won't be silently ignored and introduce undefined behavior.

Quoted 4 lines

Quoted 3 lines

Maybe we should fix the others that pass in NULL.

(no, not me :D - I'm already busy with your requested pre_plug handling)

Add it to wiki page for bite sized tasks?

Done.

Quoted 5 lines · On Fri, 8 Jun 2018 11:24:51 +0200 David Hildenbrand <[email protected]> wrote:

Quoted 5 lines

Quoted 5 lines

Quoted 4 lines

Quoted 1 line

+1 for error_abort, even if it takes another line.

+1 for error_abort
call shouldn't fail, but if does it won't be silently ignored and introduce undefined behavior.

Quoted 8 lines

Quoted 4 lines

Quoted 3 lines

Maybe we should fix the others that pass in NULL.

(no, not me :D - I'm already busy with your requested pre_plug handling)

Add it to wiki page for bite sized tasks?

Done.

FWIW, I've also added a line to check and possibly fix places where we do 'if (*errp)', which would cause QEMU to crash if the caller passes NULL.

$ git grep 'if (\*errp)'
hmp.c:    if (*errp) {
hw/ipmi/isa_ipmi_bt.c:    if (*errp)
hw/ipmi/isa_ipmi_kcs.c:    if (*errp)
hw/mem/memory-device.c:    if (*errp) {
hw/mem/memory-device.c:        if (*errp) {
hw/ppc/spapr.c:        if (*errp) {
hw/s390x/event-facility.c:        if (*errp) {
include/qapi/error.h: *     if (*errp) { // WRONG!
qga/commands-posix.c:            if (*errp) {
target/s390x/cpu_models.c:    if (*errp) {
target/s390x/cpu_models.c:        if (*errp) {
target/s390x/cpu_models.c:            if (*errp) {
target/s390x/cpu_models.c:        if (*errp) {
target/s390x/cpu_models.c:    if (*errp) {
target/s390x/cpu_models.c:    if (*errp) {
target/s390x/cpu_models.c:    if (*errp) {

target/s390x/cpu_models.c:    if (*errp) {
target/s390x/cpu_models.c:    if (*errp) {
target/s390x/cpu_models.c:    if (*errp) {
target/s390x/cpu_models.c:    if (*errp) {
target/s390x/cpu_models.c:    if (*errp) {
target/s390x/cpu_models.c:    if (*errp) {
tests/test-crypto-tlscredsx509.c:    if (*errp) {
tests/test-io-channel-tls.c:    if (*errp) {

Quoted 37 lines · Greg Kurz

Quoted 5 lines · On Fri, 8 Jun 2018 11:24:51 +0200 David Hildenbrand <[email protected]> wrote:

Quoted 5 lines

Quoted 5 lines

Quoted 4 lines

Quoted 1 line

+1 for error_abort, even if it takes another line.

+1 for error_abort
call shouldn't fail, but if does it won't be silently ignored and introduce undefined behavior.

Quoted 8 lines

Quoted 4 lines

Quoted 3 lines

Maybe we should fix the others that pass in NULL.

(no, not me :D - I'm already busy with your requested pre_plug handling)

Add it to wiki page for bite sized tasks?

Done.

FWIW, I've also added a line to check and possibly fix places where we do 'if (*errp)', which would cause QEMU to crash if the caller passes NULL.

$ git grep 'if (\*errp)'
hmp.c:    if (*errp) {
hw/ipmi/isa_ipmi_bt.c:    if (*errp)
hw/ipmi/isa_ipmi_kcs.c:    if (*errp)
hw/mem/memory-device.c:    if (*errp) {
hw/mem/memory-device.c:        if (*errp) {
hw/ppc/spapr.c:        if (*errp) {
hw/s390x/event-facility.c:        if (*errp) {
include/qapi/error.h: *     if (*errp) { // WRONG!
qga/commands-posix.c:            if (*errp) {
target/s390x/cpu_models.c:    if (*errp) {
target/s390x/cpu_models.c:        if (*errp) {
target/s390x/cpu_models.c:            if (*errp) {
target/s390x/cpu_models.c:        if (*errp) {
target/s390x/cpu_models.c:    if (*errp) {
target/s390x/cpu_models.c:    if (*errp) {
target/s390x/cpu_models.c:    if (*errp) {

Quoted 9 lines

target/s390x/cpu_models.c:    if (*errp) {
target/s390x/cpu_models.c:    if (*errp) {
target/s390x/cpu_models.c:    if (*errp) {
target/s390x/cpu_models.c:    if (*errp) {
target/s390x/cpu_models.c:    if (*errp) {
target/s390x/cpu_models.c:    if (*errp) {
tests/test-crypto-tlscredsx509.c:    if (*errp) {
tests/test-io-channel-tls.c:    if (*errp) {

I think the more important part is actually looking out for people that use NULL instead of error_abort. This way we won't silently ignore errors.

Quoted 37 lines · Greg Kurz

Quoted 5 lines · On Fri, 8 Jun 2018 11:24:51 +0200 David Hildenbrand <[email protected]> wrote:

Quoted 5 lines

Quoted 5 lines

Quoted 4 lines

Quoted 1 line

+1 for error_abort, even if it takes another line.

+1 for error_abort
call shouldn't fail, but if does it won't be silently ignored and introduce undefined behavior.

Quoted 8 lines

Quoted 4 lines

Quoted 3 lines

Maybe we should fix the others that pass in NULL.

(no, not me :D - I'm already busy with your requested pre_plug handling)

Add it to wiki page for bite sized tasks?

Done.

FWIW, I've also added a line to check and possibly fix places where we do 'if (*errp)', which would cause QEMU to crash if the caller passes NULL.

$ git grep 'if (\*errp)'
hmp.c:    if (*errp) {
hw/ipmi/isa_ipmi_bt.c:    if (*errp)
hw/ipmi/isa_ipmi_kcs.c:    if (*errp)
hw/mem/memory-device.c:    if (*errp) {
hw/mem/memory-device.c:        if (*errp) {
hw/ppc/spapr.c:        if (*errp) {
hw/s390x/event-facility.c:        if (*errp) {
include/qapi/error.h: *     if (*errp) { // WRONG!
qga/commands-posix.c:            if (*errp) {
target/s390x/cpu_models.c:    if (*errp) {
target/s390x/cpu_models.c:        if (*errp) {
target/s390x/cpu_models.c:            if (*errp) {
target/s390x/cpu_models.c:        if (*errp) {
target/s390x/cpu_models.c:    if (*errp) {
target/s390x/cpu_models.c:    if (*errp) {
target/s390x/cpu_models.c:    if (*errp) {

Quoted 9 lines

target/s390x/cpu_models.c:    if (*errp) {
target/s390x/cpu_models.c:    if (*errp) {
target/s390x/cpu_models.c:    if (*errp) {
target/s390x/cpu_models.c:    if (*errp) {
target/s390x/cpu_models.c:    if (*errp) {
target/s390x/cpu_models.c:    if (*errp) {
tests/test-crypto-tlscredsx509.c:    if (*errp) {
tests/test-io-channel-tls.c:    if (*errp) {

I think the more important part is actually looking out for people that use NULL instead of error_abort. This way we won't silently ignore errors.

Quoted 35 lines · On Fri, 8 Jun 2018 09:42:48 +0200 David Hildenbrand <[email protected]> wrote:

Quoted 33 lines · Greg Kurz

Quoted 13 lines · On Thu, 7 Jun 2018 18:52:12 +0200 David Hildenbrand <[email protected]> wrote:

The node property can always be queried and the value has already been verified in pc_dimm_realize().

Signed-off-by David Hildenbrand <[email protected]>

---
 hw/ppc/spapr.c | 9 +--------
 1 file changed, 1 insertion(+), 8 deletions(-)

hw/ppc/spapr.c +0 -0

index 2375cbee12..d038f3243e 100644

hw/ppc/spapr.c static void spapr_machine_device_plug(HotplugHandler *hotplug_dev, · line 3582

Quoted 5 lines

hw/ppc/spapr.c +0 -2

@@ -3578,14 +3578,7 @@ static void spapr_machine_device_plug(HotplugHandler *hotplug_dev,
             error_setg(errp, "Memory hotplug not supported for this machine");
             return;
         }
-        node = object_property_get_uint(OBJECT(dev), PC_DIMM_NODE_PROP, errp);
-        if (*errp) {  

Good riddance :)

hw/ppc/spapr.c static void spapr_machine_device_plug(HotplugHandler *hotplug_dev, · line 3581

Quoted 7 lines

hw/ppc/spapr.c +1 -6

-            return;
-        }
-        if (node < 0 || node >= MAX_NODES) {
-            error_setg(errp, "Invaild node %d", node);
-            return;
-        }
+        node = object_property_get_uint(OBJECT(dev), PC_DIMM_NODE_PROP, NULL);  

Maybe pass &error_abort ?

Quoted 5 lines

I'm using the same access scheme as in hw/acpi/memory_hotplug.c

("error ignored" vs. "error leads to an abort") - but this will actually never fail. But I can use error_abort here, does not matter.

Heh, /me paranoid but this is David's call and he acked that already so I guess it's okay.

Reviewed-by Greg Kurz <[email protected]>

Quoted 44 lines · Greg Kurz

Quoted 40 lines · On Fri, 8 Jun 2018 09:42:48 +0200 David Hildenbrand <[email protected]> wrote:

Quoted 32 lines · Greg Kurz

Quoted 13 lines · On Thu, 7 Jun 2018 18:52:12 +0200 David Hildenbrand <[email protected]> wrote:

The node property can always be queried and the value has already been verified in pc_dimm_realize().

Signed-off-by David Hildenbrand <[email protected]>

---
 hw/ppc/spapr.c | 9 +--------
 1 file changed, 1 insertion(+), 8 deletions(-)

hw/ppc/spapr.c +0 -0

index 2375cbee12..d038f3243e 100644

hw/ppc/spapr.c static void spapr_machine_device_plug(HotplugHandler *hotplug_dev, · line 3582

Quoted 5 lines

hw/ppc/spapr.c +0 -2

@@ -3578,14 +3578,7 @@ static void spapr_machine_device_plug(HotplugHandler *hotplug_dev,
             error_setg(errp, "Memory hotplug not supported for this machine");
             return;
         }
-        node = object_property_get_uint(OBJECT(dev), PC_DIMM_NODE_PROP, errp);
-        if (*errp) {  

Good riddance :)

hw/ppc/spapr.c static void spapr_machine_device_plug(HotplugHandler *hotplug_dev, · line 3581

Quoted 7 lines

hw/ppc/spapr.c +1 -6

-            return;
-        }
-        if (node < 0 || node >= MAX_NODES) {
-            error_setg(errp, "Invaild node %d", node);
-            return;
-        }
+        node = object_property_get_uint(OBJECT(dev), PC_DIMM_NODE_PROP, NULL);  

Quoted 1 line

Maybe pass &error_abort ?

I'm using the same access scheme as in hw/acpi/memory_hotplug.c

("error ignored" vs. "error leads to an abort") - but this will actually never fail. But I can use error_abort here, does not matter.

Quoted 2 lines

Heh, /me paranoid but this is David's call and he acked that already so I guess it's okay.

Actually, I missed this - error_abort is preferable. That's general the right choice for things that shouldn't ever fail. This way if they *do* fail we get a clear error immediately.

Quoted 2 lines

Reviewed-by Greg Kurz <[email protected]>

Let's clean the hotplug handler up by moving everything into spapr_memory_plug().

Signed-off-by David Hildenbrand <[email protected]>

---
 hw/ppc/spapr.c | 23 ++++++++++-------------
 1 file changed, 10 insertions(+), 13 deletions(-)

hw/ppc/spapr.c +10 -13

index d038f3243e..a12be24ca9 100644
@@ -3136,14 +3136,22 @@ static void spapr_add_lmbs(DeviceState *dev, uint64_t addr_start, uint64_t size,
 }
 
 static void spapr_memory_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
-                              uint32_t node, Error **errp)
+                              Error **errp)
 {
     Error *local_err = NULL;
     sPAPRMachineState *ms = SPAPR_MACHINE(hotplug_dev);
+    sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(ms);
     PCDIMMDevice *dimm = PC_DIMM(dev);
     PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm);
     MemoryRegion *mr;
     uint64_t align, size, addr;
+    uint32_t node;
+
+    if (!smc->dr_lmb_enabled) {
+        error_setg(&local_err, "Memory hotplug not supported for this machine");
+        goto out;
+    }
+    node = object_property_get_uint(OBJECT(dev), PC_DIMM_NODE_PROP, NULL);
 
     mr = ddc->get_memory_region(dimm, &local_err);
     if (local_err) {

@@ -3568,19 +3576,8 @@ out:
 static void spapr_machine_device_plug(HotplugHandler *hotplug_dev,
                                       DeviceState *dev, Error **errp)
 {
-    MachineState *ms = MACHINE(hotplug_dev);
-    sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(ms);
-
     if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
-        int node;
-
-        if (!smc->dr_lmb_enabled) {
-            error_setg(errp, "Memory hotplug not supported for this machine");
-            return;
-        }
-        node = object_property_get_uint(OBJECT(dev), PC_DIMM_NODE_PROP, NULL);
-
-        spapr_memory_plug(hotplug_dev, dev, node, errp);
+        spapr_memory_plug(hotplug_dev, dev, errp);
     } else if (object_dynamic_cast(OBJECT(dev), TYPE_SPAPR_CPU_CORE)) {
         spapr_core_plug(hotplug_dev, dev, errp);
     }

Let's clean the hotplug handler up by moving everything into spapr_memory_plug().

Signed-off-by David Hildenbrand <[email protected]>

---
 hw/ppc/spapr.c | 23 ++++++++++-------------
 1 file changed, 10 insertions(+), 13 deletions(-)

hw/ppc/spapr.c +2 -1

index d038f3243e..a12be24ca9 100644
@@ -3136,14 +3136,22 @@ static void spapr_add_lmbs(DeviceState *dev, uint64_t addr_start, uint64_t size,
 }
 
 static void spapr_memory_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
-                              uint32_t node, Error **errp)
+                              Error **errp)
 {
     Error *local_err = NULL;
     sPAPRMachineState *ms = SPAPR_MACHINE(hotplug_dev);
+    sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(ms);

hw/ppc/spapr.c +2 -12

+    node = object_property_get_uint(OBJECT(dev), PC_DIMM_NODE_PROP, NULL);
 
     mr = ddc->get_memory_region(dimm, &local_err);
     if (local_err) {

@@ -3568,19 +3576,8 @@ out:
 static void spapr_machine_device_plug(HotplugHandler *hotplug_dev,
                                       DeviceState *dev, Error **errp)
 {
-    MachineState *ms = MACHINE(hotplug_dev);
-    sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(ms);
-
     if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
-        int node;
-
-        if (!smc->dr_lmb_enabled) {
-            error_setg(errp, "Memory hotplug not supported for this machine");
-            return;
-        }
-        node = object_property_get_uint(OBJECT(dev), PC_DIMM_NODE_PROP, NULL);
-
-        spapr_memory_plug(hotplug_dev, dev, node, errp);
+        spapr_memory_plug(hotplug_dev, dev, errp);
     } else if (object_dynamic_cast(OBJECT(dev), TYPE_SPAPR_CPU_CORE)) {
         spapr_core_plug(hotplug_dev, dev, errp);
     }

Quoted 26 lines · On Thu, 7 Jun 2018 18:52:13 +0200 David Hildenbrand <[email protected]> wrote:

Let's clean the hotplug handler up by moving everything into spapr_memory_plug().

Signed-off-by David Hildenbrand <[email protected]>

---
 hw/ppc/spapr.c | 23 ++++++++++-------------
 1 file changed, 10 insertions(+), 13 deletions(-)

hw/ppc/spapr.c +2 -1

index d038f3243e..a12be24ca9 100644
@@ -3136,14 +3136,22 @@ static void spapr_add_lmbs(DeviceState *dev, uint64_t addr_start, uint64_t size,
 }
 
 static void spapr_memory_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
-                              uint32_t node, Error **errp)
+                              Error **errp)
 {
     Error *local_err = NULL;
     sPAPRMachineState *ms = SPAPR_MACHINE(hotplug_dev);
+    sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(ms);
     PCDIMMDevice *dimm = PC_DIMM(dev);
     PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm);
     MemoryRegion *mr;
     uint64_t align, size, addr;

hw/ppc/spapr.c static void spapr_add_lmbs(DeviceState *dev, uint64_t addr_start, uint64_t size, · line 3153

Quoted 6 lines

hw/ppc/spapr.c +6 -0

+    uint32_t node;
+
+    if (!smc->dr_lmb_enabled) {
+        error_setg(&local_err, "Memory hotplug not supported for this machine");
+        goto out;
+    }

Wouldn't it be more appropriate to move this check to spapr_memory_pre_plug() ?

Quoted 26 lines

hw/ppc/spapr.c +2 -12

+    node = object_property_get_uint(OBJECT(dev), PC_DIMM_NODE_PROP, NULL);
 
     mr = ddc->get_memory_region(dimm, &local_err);
     if (local_err) {

@@ -3568,19 +3576,8 @@ out:
 static void spapr_machine_device_plug(HotplugHandler *hotplug_dev,
                                       DeviceState *dev, Error **errp)
 {
-    MachineState *ms = MACHINE(hotplug_dev);
-    sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(ms);
-
     if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
-        int node;
-
-        if (!smc->dr_lmb_enabled) {
-            error_setg(errp, "Memory hotplug not supported for this machine");
-            return;
-        }
-        node = object_property_get_uint(OBJECT(dev), PC_DIMM_NODE_PROP, NULL);
-
-        spapr_memory_plug(hotplug_dev, dev, node, errp);
+        spapr_memory_plug(hotplug_dev, dev, errp);
     } else if (object_dynamic_cast(OBJECT(dev), TYPE_SPAPR_CPU_CORE)) {
         spapr_core_plug(hotplug_dev, dev, errp);
     }

Quoted 29 lines · Greg Kurz

Quoted 26 lines · On Thu, 7 Jun 2018 18:52:13 +0200 David Hildenbrand <[email protected]> wrote:

Let's clean the hotplug handler up by moving everything into spapr_memory_plug().

Signed-off-by David Hildenbrand <[email protected]>

---
 hw/ppc/spapr.c | 23 ++++++++++-------------
 1 file changed, 10 insertions(+), 13 deletions(-)

hw/ppc/spapr.c +2 -1

index d038f3243e..a12be24ca9 100644
@@ -3136,14 +3136,22 @@ static void spapr_add_lmbs(DeviceState *dev, uint64_t addr_start, uint64_t size,
 }
 
 static void spapr_memory_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
-                              uint32_t node, Error **errp)
+                              Error **errp)
 {
     Error *local_err = NULL;
     sPAPRMachineState *ms = SPAPR_MACHINE(hotplug_dev);
+    sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(ms);
     PCDIMMDevice *dimm = PC_DIMM(dev);
     PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm);
     MemoryRegion *mr;
     uint64_t align, size, addr;

Quoted 8 lines

hw/ppc/spapr.c static void spapr_add_lmbs(DeviceState *dev, uint64_t addr_start, uint64_t size, · line 3153

Quoted 6 lines

hw/ppc/spapr.c +6 -0

+    uint32_t node;
+
+    if (!smc->dr_lmb_enabled) {
+        error_setg(&local_err, "Memory hotplug not supported for this machine");
+        goto out;
+    }  

Wouldn't it be more appropriate to move this check to spapr_memory_pre_plug() ?

Think you're right (and as spapr_memory_pre_plug() already exists, it's easy), other opinions? Thanks.

Quoted 28 lines

Quoted 27 lines

hw/ppc/spapr.c +2 -12

+    node = object_property_get_uint(OBJECT(dev), PC_DIMM_NODE_PROP, NULL);
 
     mr = ddc->get_memory_region(dimm, &local_err);
     if (local_err) {

@@ -3568,19 +3576,8 @@ out:
 static void spapr_machine_device_plug(HotplugHandler *hotplug_dev,
                                       DeviceState *dev, Error **errp)
 {
-    MachineState *ms = MACHINE(hotplug_dev);
-    sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(ms);
-
     if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
-        int node;
-
-        if (!smc->dr_lmb_enabled) {
-            error_setg(errp, "Memory hotplug not supported for this machine");
-            return;
-        }
-        node = object_property_get_uint(OBJECT(dev), PC_DIMM_NODE_PROP, NULL);
-
-        spapr_memory_plug(hotplug_dev, dev, node, errp);
+        spapr_memory_plug(hotplug_dev, dev, errp);
     } else if (object_dynamic_cast(OBJECT(dev), TYPE_SPAPR_CPU_CORE)) {
         spapr_core_plug(hotplug_dev, dev, errp);
     }  

Let's introduce and use local error variables in the hotplug handler functions.

Signed-off-by David Hildenbrand <[email protected]>

---
 hw/ppc/spapr.c | 29 ++++++++++++++++++++---------
 1 file changed, 20 insertions(+), 9 deletions(-)

hw/ppc/spapr.c +20 -9

index a12be24ca9..4447cb197f 100644
@@ -3576,11 +3576,14 @@ out:
 static void spapr_machine_device_plug(HotplugHandler *hotplug_dev,
                                       DeviceState *dev, Error **errp)
 {
+    Error *local_err = NULL;
+
     if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
-        spapr_memory_plug(hotplug_dev, dev, errp);
+        spapr_memory_plug(hotplug_dev, dev, &local_err);
     } else if (object_dynamic_cast(OBJECT(dev), TYPE_SPAPR_CPU_CORE)) {
-        spapr_core_plug(hotplug_dev, dev, errp);
+        spapr_core_plug(hotplug_dev, dev, &local_err);
     }
+    error_propagate(errp, local_err);
 }
 
 static void spapr_machine_device_unplug_request(HotplugHandler *hotplug_dev,

@@ -3588,10 +3591,11 @@ static void spapr_machine_device_unplug_request(HotplugHandler *hotplug_dev,
 {
     sPAPRMachineState *sms = SPAPR_MACHINE(OBJECT(hotplug_dev));
     MachineClass *mc = MACHINE_GET_CLASS(sms);
+    Error *local_err = NULL;
 
     if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
         if (spapr_ovec_test(sms->ov5_cas, OV5_HP_EVT)) {
-            spapr_memory_unplug_request(hotplug_dev, dev, errp);
+            spapr_memory_unplug_request(hotplug_dev, dev, &local_err);
         } else {
             /* NOTE: this means there is a window after guest reset, prior to
              * CAS negotiation, where unplug requests will fail due to the

@@ -3599,25 +3603,32 @@ static void spapr_machine_device_unplug_request(HotplugHandler *hotplug_dev,
              * the case with PCI unplug, where the events will be queued and
              * eventually handled by the guest after boot
              */
-            error_setg(errp, "Memory hot unplug not supported for this guest");
+            error_setg(&local_err,
+                       "Memory hot unplug not supported for this guest");
         }
     } else if (object_dynamic_cast(OBJECT(dev), TYPE_SPAPR_CPU_CORE)) {
         if (!mc->has_hotpluggable_cpus) {
-            error_setg(errp, "CPU hot unplug not supported on this machine");
-            return;
+            error_setg(&local_err,
+                       "CPU hot unplug not supported on this machine");
+            goto out;
         }
-        spapr_core_unplug_request(hotplug_dev, dev, errp);
+        spapr_core_unplug_request(hotplug_dev, dev, &local_err);
     }
+out:
+    error_propagate(errp, local_err);
 }
 
 static void spapr_machine_device_pre_plug(HotplugHandler *hotplug_dev,
                                           DeviceState *dev, Error **errp)
 {
+    Error *local_err = NULL;
+
     if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
-        spapr_memory_pre_plug(hotplug_dev, dev, errp);
+        spapr_memory_pre_plug(hotplug_dev, dev, &local_err);
     } else if (object_dynamic_cast(OBJECT(dev), TYPE_SPAPR_CPU_CORE)) {
-        spapr_core_pre_plug(hotplug_dev, dev, errp);
+        spapr_core_pre_plug(hotplug_dev, dev, &local_err);
     }
+    error_propagate(errp, local_err);
 }
 
 static HotplugHandler *spapr_get_hotplug_handler(MachineState *machine,

Let's introduce and use local error variables in the hotplug handler functions.

Signed-off-by David Hildenbrand <[email protected]>

---

 hw/ppc/spapr.c | 29 ++++++++++++++++++++---------
 1 file changed, 20 insertions(+), 9 deletions(-)

hw/ppc/spapr.c +20 -9

index a12be24ca9..4447cb197f 100644
@@ -3576,11 +3576,14 @@ out:
 static void spapr_machine_device_plug(HotplugHandler *hotplug_dev,
                                       DeviceState *dev, Error **errp)
 {
+    Error *local_err = NULL;
+
     if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
-        spapr_memory_plug(hotplug_dev, dev, errp);
+        spapr_memory_plug(hotplug_dev, dev, &local_err);
     } else if (object_dynamic_cast(OBJECT(dev), TYPE_SPAPR_CPU_CORE)) {
-        spapr_core_plug(hotplug_dev, dev, errp);
+        spapr_core_plug(hotplug_dev, dev, &local_err);
     }
+    error_propagate(errp, local_err);
 }
 
 static void spapr_machine_device_unplug_request(HotplugHandler *hotplug_dev,

@@ -3588,10 +3591,11 @@ static void spapr_machine_device_unplug_request(HotplugHandler *hotplug_dev,
 {
     sPAPRMachineState *sms = SPAPR_MACHINE(OBJECT(hotplug_dev));
     MachineClass *mc = MACHINE_GET_CLASS(sms);
+    Error *local_err = NULL;
 
     if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
         if (spapr_ovec_test(sms->ov5_cas, OV5_HP_EVT)) {
-            spapr_memory_unplug_request(hotplug_dev, dev, errp);
+            spapr_memory_unplug_request(hotplug_dev, dev, &local_err);
         } else {
             /* NOTE: this means there is a window after guest reset, prior to
              * CAS negotiation, where unplug requests will fail due to the

@@ -3599,25 +3603,32 @@ static void spapr_machine_device_unplug_request(HotplugHandler *hotplug_dev,
              * the case with PCI unplug, where the events will be queued and
              * eventually handled by the guest after boot
              */
-            error_setg(errp, "Memory hot unplug not supported for this guest");
+            error_setg(&local_err,
+                       "Memory hot unplug not supported for this guest");
         }
     } else if (object_dynamic_cast(OBJECT(dev), TYPE_SPAPR_CPU_CORE)) {
         if (!mc->has_hotpluggable_cpus) {
-            error_setg(errp, "CPU hot unplug not supported on this machine");
-            return;
+            error_setg(&local_err,
+                       "CPU hot unplug not supported on this machine");
+            goto out;
         }
-        spapr_core_unplug_request(hotplug_dev, dev, errp);
+        spapr_core_unplug_request(hotplug_dev, dev, &local_err);
     }
+out:
+    error_propagate(errp, local_err);
 }
 
 static void spapr_machine_device_pre_plug(HotplugHandler *hotplug_dev,
                                           DeviceState *dev, Error **errp)
 {
+    Error *local_err = NULL;
+
     if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
-        spapr_memory_pre_plug(hotplug_dev, dev, errp);
+        spapr_memory_pre_plug(hotplug_dev, dev, &local_err);
     } else if (object_dynamic_cast(OBJECT(dev), TYPE_SPAPR_CPU_CORE)) {
-        spapr_core_pre_plug(hotplug_dev, dev, errp);
+        spapr_core_pre_plug(hotplug_dev, dev, &local_err);
     }
+    error_propagate(errp, local_err);
 }
 
 static HotplugHandler *spapr_get_hotplug_handler(MachineState *machine,

We'll be handling unplug of e.g. CPUs and PCDIMMs via the general hotplug handler soon, so let's add that handler function.

Signed-off-by David Hildenbrand <[email protected]>

---
 hw/ppc/spapr.c | 6 ++++++
 1 file changed, 6 insertions(+)

hw/ppc/spapr.c +6 -0

index 4447cb197f..bcb72d9fa7 100644
@@ -3586,6 +3586,11 @@ static void spapr_machine_device_plug(HotplugHandler *hotplug_dev,
     error_propagate(errp, local_err);
 }
 
+static void spapr_machine_device_unplug(HotplugHandler *hotplug_dev,
+                                        DeviceState *dev, Error **errp)
+{
+}
+
 static void spapr_machine_device_unplug_request(HotplugHandler *hotplug_dev,
                                                 DeviceState *dev, Error **errp)
 {

@@ -3988,6 +3993,7 @@ static void spapr_machine_class_init(ObjectClass *oc, void *data)
     mc->get_default_cpu_node_id = spapr_get_default_cpu_node_id;
     mc->possible_cpu_arch_ids = spapr_possible_cpu_arch_ids;
     hc->unplug_request = spapr_machine_device_unplug_request;
+    hc->unplug = spapr_machine_device_unplug;
 
     smc->dr_lmb_enabled = true;
     mc->default_cpu_type = POWERPC_CPU_TYPE_NAME("power8_v2.0");

We'll be handling unplug of e.g. CPUs and PCDIMMs via the general hotplug handler soon, so let's add that handler function.

Signed-off-by David Hildenbrand <[email protected]>

---
 hw/ppc/spapr.c | 6 ++++++
 1 file changed, 6 insertions(+)

hw/ppc/spapr.c +6 -0

index 4447cb197f..bcb72d9fa7 100644
@@ -3586,6 +3586,11 @@ static void spapr_machine_device_plug(HotplugHandler *hotplug_dev,
     error_propagate(errp, local_err);
 }
 
+static void spapr_machine_device_unplug(HotplugHandler *hotplug_dev,
+                                        DeviceState *dev, Error **errp)
+{
+}
+
 static void spapr_machine_device_unplug_request(HotplugHandler *hotplug_dev,
                                                 DeviceState *dev, Error **errp)
 {

@@ -3988,6 +3993,7 @@ static void spapr_machine_class_init(ObjectClass *oc, void *data)
     mc->get_default_cpu_node_id = spapr_get_default_cpu_node_id;
     mc->possible_cpu_arch_ids = spapr_possible_cpu_arch_ids;
     hc->unplug_request = spapr_machine_device_unplug_request;
+    hc->unplug = spapr_machine_device_unplug;
 
     smc->dr_lmb_enabled = true;
     mc->default_cpu_type = POWERPC_CPU_TYPE_NAME("power8_v2.0");

We'll be handling unplug of e.g. CPUs and PCDIMMs via the general hotplug handler soon, so let's add that handler function.

Signed-off-by David Hildenbrand <[email protected]>

---

 hw/ppc/spapr.c | 6 ++++++
 1 file changed, 6 insertions(+)

hw/ppc/spapr.c +6 -0

index 4447cb197f..bcb72d9fa7 100644
@@ -3586,6 +3586,11 @@ static void spapr_machine_device_plug(HotplugHandler *hotplug_dev,
     error_propagate(errp, local_err);
 }
 
+static void spapr_machine_device_unplug(HotplugHandler *hotplug_dev,
+                                        DeviceState *dev, Error **errp)
+{
+}
+
 static void spapr_machine_device_unplug_request(HotplugHandler *hotplug_dev,
                                                 DeviceState *dev, Error **errp)
 {

@@ -3988,6 +3993,7 @@ static void spapr_machine_class_init(ObjectClass *oc, void *data)
     mc->get_default_cpu_node_id = spapr_get_default_cpu_node_id;
     mc->possible_cpu_arch_ids = spapr_possible_cpu_arch_ids;
     hc->unplug_request = spapr_machine_device_unplug_request;
+    hc->unplug = spapr_machine_device_unplug;
 
     smc->dr_lmb_enabled = true;
     mc->default_cpu_type = POWERPC_CPU_TYPE_NAME("power8_v2.0");

Let's handle it via hotplug_handler_unplug(). E.g. necessary to hotplug/ unplug memory devices (which a pc-dimm is) later.

Signed-off-by David Hildenbrand <[email protected]>

---
 hw/ppc/spapr.c | 18 +++++++++++++++---
 1 file changed, 15 insertions(+), 3 deletions(-)

hw/ppc/spapr.c +15 -3

index bcb72d9fa7..0a8a3455d6 100644
@@ -3298,7 +3298,8 @@ static sPAPRDIMMState *spapr_recover_pending_dimm_state(sPAPRMachineState *ms,
 /* Callback to be called during DRC release. */
 void spapr_lmb_release(DeviceState *dev)
 {
-    sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_hotplug_handler(dev));
+    HotplugHandler *hotplug_ctrl = qdev_get_hotplug_handler(dev);
+    sPAPRMachineState *spapr = SPAPR_MACHINE(hotplug_ctrl);
     sPAPRDIMMState *ds = spapr_pending_dimm_unplugs_find(spapr, PC_DIMM(dev));
 
     /* This information will get lost if a migration occurs

@@ -3316,9 +3317,17 @@ void spapr_lmb_release(DeviceState *dev)
 
     /*
      * Now that all the LMBs have been removed by the guest, call the
-     * pc-dimm unplug handler to cleanup up the pc-dimm device.
+     * unplug handler chain. This can never fail.
      */
-    pc_dimm_memory_unplug(dev, MACHINE(spapr));
+    hotplug_handler_unplug(hotplug_ctrl, dev, &error_abort);
+}
+
+static void spapr_memory_unplug(HotplugHandler *hotplug_dev, DeviceState *dev)
+{
+    sPAPRMachineState *spapr = SPAPR_MACHINE(hotplug_dev);
+    sPAPRDIMMState *ds = spapr_pending_dimm_unplugs_find(spapr, PC_DIMM(dev));
+
+    pc_dimm_memory_unplug(dev, MACHINE(hotplug_dev));
     object_unparent(OBJECT(dev));
     spapr_pending_dimm_unplugs_remove(spapr, ds);
 }

@@ -3589,6 +3598,9 @@ static void spapr_machine_device_plug(HotplugHandler *hotplug_dev,
 static void spapr_machine_device_unplug(HotplugHandler *hotplug_dev,
                                         DeviceState *dev, Error **errp)
 {
+    if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
+        spapr_memory_unplug(hotplug_dev, dev);
+    }
 }
 
 static void spapr_machine_device_unplug_request(HotplugHandler *hotplug_dev,

Let's handle it via hotplug_handler_unplug(). E.g. necessary to hotplug/ unplug memory devices (which a pc-dimm is) later.

Signed-off-by David Hildenbrand <[email protected]>

---
 hw/ppc/spapr.c | 18 +++++++++++++++---
 1 file changed, 15 insertions(+), 3 deletions(-)

hw/ppc/spapr.c +15 -3

index bcb72d9fa7..0a8a3455d6 100644
@@ -3298,7 +3298,8 @@ static sPAPRDIMMState *spapr_recover_pending_dimm_state(sPAPRMachineState *ms,
 /* Callback to be called during DRC release. */
 void spapr_lmb_release(DeviceState *dev)
 {
-    sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_hotplug_handler(dev));
+    HotplugHandler *hotplug_ctrl = qdev_get_hotplug_handler(dev);
+    sPAPRMachineState *spapr = SPAPR_MACHINE(hotplug_ctrl);
     sPAPRDIMMState *ds = spapr_pending_dimm_unplugs_find(spapr, PC_DIMM(dev));
 
     /* This information will get lost if a migration occurs

@@ -3316,9 +3317,17 @@ void spapr_lmb_release(DeviceState *dev)
 
     /*
      * Now that all the LMBs have been removed by the guest, call the
-     * pc-dimm unplug handler to cleanup up the pc-dimm device.
+     * unplug handler chain. This can never fail.
      */
-    pc_dimm_memory_unplug(dev, MACHINE(spapr));
+    hotplug_handler_unplug(hotplug_ctrl, dev, &error_abort);
+}
+
+static void spapr_memory_unplug(HotplugHandler *hotplug_dev, DeviceState *dev)
+{
+    sPAPRMachineState *spapr = SPAPR_MACHINE(hotplug_dev);
+    sPAPRDIMMState *ds = spapr_pending_dimm_unplugs_find(spapr, PC_DIMM(dev));
+
+    pc_dimm_memory_unplug(dev, MACHINE(hotplug_dev));
     object_unparent(OBJECT(dev));
     spapr_pending_dimm_unplugs_remove(spapr, ds);
 }

@@ -3589,6 +3598,9 @@ static void spapr_machine_device_plug(HotplugHandler *hotplug_dev,
 static void spapr_machine_device_unplug(HotplugHandler *hotplug_dev,
                                         DeviceState *dev, Error **errp)
 {
+    if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
+        spapr_memory_unplug(hotplug_dev, dev);
+    }
 }
 
 static void spapr_machine_device_unplug_request(HotplugHandler *hotplug_dev,

Quoted 2 lines · On Thu, 7 Jun 2018 18:52:16 +0200 David Hildenbrand <[email protected]> wrote:

Let's handle it via hotplug_handler_unplug(). E.g. necessary to hotplug/ unplug memory devices (which a pc-dimm is) later.

Perhaps something like following would be better:

Factor out memory unplug into separate function from spapr_lmb_release(). Then use generic hotplug_handler_unplug() to trigger memory unplug, which would call spapr_machine_device_unplug() -> spapr_memory_unplug() in the end .
This way unplug operation is not buried in lmb internals and located in the same place like in other targets, following similar logic/call chain across targets.

Quoted 51 lines

Signed-off-by David Hildenbrand <[email protected]>

---
 hw/ppc/spapr.c | 18 +++++++++++++++---
 1 file changed, 15 insertions(+), 3 deletions(-)

hw/ppc/spapr.c +15 -3

index bcb72d9fa7..0a8a3455d6 100644
@@ -3298,7 +3298,8 @@ static sPAPRDIMMState *spapr_recover_pending_dimm_state(sPAPRMachineState *ms,
 /* Callback to be called during DRC release. */
 void spapr_lmb_release(DeviceState *dev)
 {
-    sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_hotplug_handler(dev));
+    HotplugHandler *hotplug_ctrl = qdev_get_hotplug_handler(dev);
+    sPAPRMachineState *spapr = SPAPR_MACHINE(hotplug_ctrl);
     sPAPRDIMMState *ds = spapr_pending_dimm_unplugs_find(spapr, PC_DIMM(dev));
 
     /* This information will get lost if a migration occurs

@@ -3316,9 +3317,17 @@ void spapr_lmb_release(DeviceState *dev)
 
     /*
      * Now that all the LMBs have been removed by the guest, call the
-     * pc-dimm unplug handler to cleanup up the pc-dimm device.
+     * unplug handler chain. This can never fail.
      */
-    pc_dimm_memory_unplug(dev, MACHINE(spapr));
+    hotplug_handler_unplug(hotplug_ctrl, dev, &error_abort);
+}
+
+static void spapr_memory_unplug(HotplugHandler *hotplug_dev, DeviceState *dev)
+{
+    sPAPRMachineState *spapr = SPAPR_MACHINE(hotplug_dev);
+    sPAPRDIMMState *ds = spapr_pending_dimm_unplugs_find(spapr, PC_DIMM(dev));
+
+    pc_dimm_memory_unplug(dev, MACHINE(hotplug_dev));
     object_unparent(OBJECT(dev));
     spapr_pending_dimm_unplugs_remove(spapr, ds);
 }

@@ -3589,6 +3598,9 @@ static void spapr_machine_device_plug(HotplugHandler *hotplug_dev,
 static void spapr_machine_device_unplug(HotplugHandler *hotplug_dev,
                                         DeviceState *dev, Error **errp)
 {
+    if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
+        spapr_memory_unplug(hotplug_dev, dev);
+    }
 }
 
 static void spapr_machine_device_unplug_request(HotplugHandler *hotplug_dev,

Quoted 7 lines · Igor Mammedov

Quoted 2 lines · On Thu, 7 Jun 2018 18:52:16 +0200 David Hildenbrand <[email protected]> wrote:

Let's handle it via hotplug_handler_unplug(). E.g. necessary to hotplug/ unplug memory devices (which a pc-dimm is) later.

Perhaps something like following would be better:

Quoted 7 lines

Factor out memory unplug into separate function from spapr_lmb_release(). Then use generic hotplug_handler_unplug() to trigger memory unplug, which would call spapr_machine_device_unplug() -> spapr_memory_unplug() in the end .
This way unplug operation is not buried in lmb internals and located in the same place like in other targets, following similar logic/call chain across targets.

Can this be an addon patch? Sounds like factoring out more and moving more.

Quoted 50 lines

Quoted 49 lines

Signed-off-by David Hildenbrand <[email protected]>

---
 hw/ppc/spapr.c | 18 +++++++++++++++---
 1 file changed, 15 insertions(+), 3 deletions(-)

hw/ppc/spapr.c +15 -3

index bcb72d9fa7..0a8a3455d6 100644
@@ -3298,7 +3298,8 @@ static sPAPRDIMMState *spapr_recover_pending_dimm_state(sPAPRMachineState *ms,
 /* Callback to be called during DRC release. */
 void spapr_lmb_release(DeviceState *dev)
 {
-    sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_hotplug_handler(dev));
+    HotplugHandler *hotplug_ctrl = qdev_get_hotplug_handler(dev);
+    sPAPRMachineState *spapr = SPAPR_MACHINE(hotplug_ctrl);
     sPAPRDIMMState *ds = spapr_pending_dimm_unplugs_find(spapr, PC_DIMM(dev));
 
     /* This information will get lost if a migration occurs

@@ -3316,9 +3317,17 @@ void spapr_lmb_release(DeviceState *dev)
 
     /*
      * Now that all the LMBs have been removed by the guest, call the
-     * pc-dimm unplug handler to cleanup up the pc-dimm device.
+     * unplug handler chain. This can never fail.
      */
-    pc_dimm_memory_unplug(dev, MACHINE(spapr));
+    hotplug_handler_unplug(hotplug_ctrl, dev, &error_abort);
+}
+
+static void spapr_memory_unplug(HotplugHandler *hotplug_dev, DeviceState *dev)
+{
+    sPAPRMachineState *spapr = SPAPR_MACHINE(hotplug_dev);
+    sPAPRDIMMState *ds = spapr_pending_dimm_unplugs_find(spapr, PC_DIMM(dev));
+
+    pc_dimm_memory_unplug(dev, MACHINE(hotplug_dev));
     object_unparent(OBJECT(dev));
     spapr_pending_dimm_unplugs_remove(spapr, ds);
 }

@@ -3589,6 +3598,9 @@ static void spapr_machine_device_plug(HotplugHandler *hotplug_dev,
 static void spapr_machine_device_unplug(HotplugHandler *hotplug_dev,
                                         DeviceState *dev, Error **errp)
 {
+    if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
+        spapr_memory_unplug(hotplug_dev, dev);
+    }
 }
 
 static void spapr_machine_device_unplug_request(HotplugHandler *hotplug_dev,  

Quoted 16 lines · On Fri, 8 Jun 2018 11:02:23 +0200 David Hildenbrand <[email protected]> wrote:

Quoted 14 lines · Igor Mammedov

Quoted 2 lines · On Thu, 7 Jun 2018 18:52:16 +0200 David Hildenbrand <[email protected]> wrote:

Let's handle it via hotplug_handler_unplug(). E.g. necessary to hotplug/ unplug memory devices (which a pc-dimm is) later.

Perhaps something like following would be better:

Factor out memory unplug into separate function from spapr_lmb_release(). Then use generic hotplug_handler_unplug() to trigger memory unplug, which would call spapr_machine_device_unplug() -> spapr_memory_unplug() in the end .
This way unplug operation is not buried in lmb internals and located in the same place like in other targets, following similar logic/call chain across targets.

Quoted 1 line

Can this be an addon patch? Sounds like factoring out more and moving more.

I've suggested ^^^ it as this patch description instead of the current one that doesn't really makes the sense on it's own.

Let's handle it via hotplug_handler_unplug(). E.g. necessary to hotplug/ unplug memory devices (which a pc-dimm is) later.

Signed-off-by David Hildenbrand <[email protected]>

---

 hw/ppc/spapr.c | 18 +++++++++++++++---
 1 file changed, 15 insertions(+), 3 deletions(-)

hw/ppc/spapr.c +15 -3

index bcb72d9fa7..0a8a3455d6 100644
@@ -3298,7 +3298,8 @@ static sPAPRDIMMState *spapr_recover_pending_dimm_state(sPAPRMachineState *ms,
 /* Callback to be called during DRC release. */
 void spapr_lmb_release(DeviceState *dev)
 {
-    sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_hotplug_handler(dev));
+    HotplugHandler *hotplug_ctrl = qdev_get_hotplug_handler(dev);
+    sPAPRMachineState *spapr = SPAPR_MACHINE(hotplug_ctrl);
     sPAPRDIMMState *ds = spapr_pending_dimm_unplugs_find(spapr, PC_DIMM(dev));
 
     /* This information will get lost if a migration occurs

@@ -3316,9 +3317,17 @@ void spapr_lmb_release(DeviceState *dev)
 
     /*
      * Now that all the LMBs have been removed by the guest, call the
-     * pc-dimm unplug handler to cleanup up the pc-dimm device.
+     * unplug handler chain. This can never fail.
      */
-    pc_dimm_memory_unplug(dev, MACHINE(spapr));
+    hotplug_handler_unplug(hotplug_ctrl, dev, &error_abort);
+}
+
+static void spapr_memory_unplug(HotplugHandler *hotplug_dev, DeviceState *dev)
+{
+    sPAPRMachineState *spapr = SPAPR_MACHINE(hotplug_dev);
+    sPAPRDIMMState *ds = spapr_pending_dimm_unplugs_find(spapr, PC_DIMM(dev));
+
+    pc_dimm_memory_unplug(dev, MACHINE(hotplug_dev));
     object_unparent(OBJECT(dev));
     spapr_pending_dimm_unplugs_remove(spapr, ds);
 }

@@ -3589,6 +3598,9 @@ static void spapr_machine_device_plug(HotplugHandler *hotplug_dev,
 static void spapr_machine_device_unplug(HotplugHandler *hotplug_dev,
                                         DeviceState *dev, Error **errp)
 {
+    if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
+        spapr_memory_unplug(hotplug_dev, dev);
+    }
 }
 
 static void spapr_machine_device_unplug_request(HotplugHandler *hotplug_dev,

Let's handle it via hotplug_handler_unplug() to make plug/unplug code look symmetrical.

Acked-by Igor Mammedov <[email protected]>

Signed-off-by David Hildenbrand <[email protected]>

---
 hw/ppc/spapr.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

hw/ppc/spapr.c +11 -1

index 0a8a3455d6..994deea8cf 100644
@@ -3415,7 +3415,15 @@ static void *spapr_populate_hotplug_cpu_dt(CPUState *cs, int *fdt_offset,
 /* Callback to be called during DRC release. */
 void spapr_core_release(DeviceState *dev)
 {
-    MachineState *ms = MACHINE(qdev_get_hotplug_handler(dev));
+    HotplugHandler *hotplug_ctrl = qdev_get_hotplug_handler(dev);
+
+    /* Call the unplug handler chain. This can never fail. */
+    hotplug_handler_unplug(hotplug_ctrl, dev, &error_abort);
+}
+
+static void spapr_core_unplug(HotplugHandler *hotplug_dev, DeviceState *dev)
+{
+    MachineState *ms = MACHINE(hotplug_dev);
     sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(ms);
     CPUCore *cc = CPU_CORE(dev);
     CPUArchId *core_slot = spapr_find_cpu_slot(ms, cc->core_id, NULL);

@@ -3600,6 +3608,8 @@ static void spapr_machine_device_unplug(HotplugHandler *hotplug_dev,
 {
     if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
         spapr_memory_unplug(hotplug_dev, dev);
+    } else if (object_dynamic_cast(OBJECT(dev), TYPE_SPAPR_CPU_CORE)) {
+        spapr_core_unplug(hotplug_dev, dev);
     }
 }
 

Let's handle it via hotplug_handler_unplug() to make plug/unplug code look symmetrical.

Acked-by Igor Mammedov <[email protected]>

Signed-off-by David Hildenbrand <[email protected]>

---
 hw/ppc/spapr.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

hw/ppc/spapr.c +11 -1

index 0a8a3455d6..994deea8cf 100644
@@ -3415,7 +3415,15 @@ static void *spapr_populate_hotplug_cpu_dt(CPUState *cs, int *fdt_offset,
 /* Callback to be called during DRC release. */
 void spapr_core_release(DeviceState *dev)
 {
-    MachineState *ms = MACHINE(qdev_get_hotplug_handler(dev));
+    HotplugHandler *hotplug_ctrl = qdev_get_hotplug_handler(dev);
+
+    /* Call the unplug handler chain. This can never fail. */
+    hotplug_handler_unplug(hotplug_ctrl, dev, &error_abort);
+}
+
+static void spapr_core_unplug(HotplugHandler *hotplug_dev, DeviceState *dev)
+{
+    MachineState *ms = MACHINE(hotplug_dev);
     sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(ms);
     CPUCore *cc = CPU_CORE(dev);
     CPUArchId *core_slot = spapr_find_cpu_slot(ms, cc->core_id, NULL);

@@ -3600,6 +3608,8 @@ static void spapr_machine_device_unplug(HotplugHandler *hotplug_dev,
 {
     if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
         spapr_memory_unplug(hotplug_dev, dev);
+    } else if (object_dynamic_cast(OBJECT(dev), TYPE_SPAPR_CPU_CORE)) {
+        spapr_core_unplug(hotplug_dev, dev);
     }
 }
 

Let's handle it via hotplug_handler_unplug() to make plug/unplug code look symmetrical.

Acked-by Igor Mammedov <[email protected]>

Signed-off-by David Hildenbrand <[email protected]>

---

 hw/ppc/spapr.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

hw/ppc/spapr.c +11 -1

index 0a8a3455d6..994deea8cf 100644
@@ -3415,7 +3415,15 @@ static void *spapr_populate_hotplug_cpu_dt(CPUState *cs, int *fdt_offset,
 /* Callback to be called during DRC release. */
 void spapr_core_release(DeviceState *dev)
 {
-    MachineState *ms = MACHINE(qdev_get_hotplug_handler(dev));
+    HotplugHandler *hotplug_ctrl = qdev_get_hotplug_handler(dev);
+
+    /* Call the unplug handler chain. This can never fail. */
+    hotplug_handler_unplug(hotplug_ctrl, dev, &error_abort);
+}
+
+static void spapr_core_unplug(HotplugHandler *hotplug_dev, DeviceState *dev)
+{
+    MachineState *ms = MACHINE(hotplug_dev);
     sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(ms);
     CPUCore *cc = CPU_CORE(dev);
     CPUArchId *core_slot = spapr_find_cpu_slot(ms, cc->core_id, NULL);

@@ -3600,6 +3608,8 @@ static void spapr_machine_device_unplug(HotplugHandler *hotplug_dev,
 {
     if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
         spapr_memory_unplug(hotplug_dev, dev);
+    } else if (object_dynamic_cast(OBJECT(dev), TYPE_SPAPR_CPU_CORE)) {
+        spapr_core_unplug(hotplug_dev, dev);
     }
 }
 

Let's introduce and use local error variables in the hotplug handler functions.

Signed-off-by David Hildenbrand <[email protected]>

---
 hw/s390x/s390-virtio-ccw.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

hw/s390x/s390-virtio-ccw.c +5 -1

index 7ae5fb38dd..29ea50a177 100644
@@ -434,18 +434,23 @@ static void s390_machine_reset(void)
 static void s390_machine_device_plug(HotplugHandler *hotplug_dev,
                                      DeviceState *dev, Error **errp)
 {
+    Error *local_err = NULL;
+
     if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
-        s390_cpu_plug(hotplug_dev, dev, errp);
+        s390_cpu_plug(hotplug_dev, dev, &local_err);
     }
+    error_propagate(errp, local_err);
 }
 
 static void s390_machine_device_unplug_request(HotplugHandler *hotplug_dev,
                                                DeviceState *dev, Error **errp)
 {
+    Error *local_err = NULL;

Quoted 38 lines · On Thu, 7 Jun 2018 18:52:18 +0200 David Hildenbrand <[email protected]> wrote:

Let's introduce and use local error variables in the hotplug handler functions.

Signed-off-by David Hildenbrand <[email protected]>

---
 hw/s390x/s390-virtio-ccw.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

hw/s390x/s390-virtio-ccw.c +8 -3

index 7ae5fb38dd..29ea50a177 100644
@@ -434,18 +434,23 @@ static void s390_machine_reset(void)
 static void s390_machine_device_plug(HotplugHandler *hotplug_dev,
                                      DeviceState *dev, Error **errp)
 {
+    Error *local_err = NULL;
+
     if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
-        s390_cpu_plug(hotplug_dev, dev, errp);
+        s390_cpu_plug(hotplug_dev, dev, &local_err);
     }
+    error_propagate(errp, local_err);
 }
 
 static void s390_machine_device_unplug_request(HotplugHandler *hotplug_dev,
                                                DeviceState *dev, Error **errp)
 {
+    Error *local_err = NULL;
+
     if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
-        error_setg(errp, "CPU hot unplug not supported on this machine");
-        return;
+        error_setg(&local_err, "CPU hot unplug not supported on this machine");
     }
+    error_propagate(errp, local_err);
 }
 

hw/s390x/s390-virtio-ccw.c static void s390_machine_reset(void) · line 456

Quoted 1 line

hw/s390x/s390-virtio-ccw.c +0 -0

 static CpuInstanceProperties s390_cpu_index_to_props(MachineState *ms,

Just seeing this patch by itself, it does not really make much sense. Even if this is a split out clean-up series, I'd prefer this to go together with a patch that actually adds something more to the plug/unplug functions.

Quoted 41 lines · Cornelia Huck

Quoted 38 lines · On Thu, 7 Jun 2018 18:52:18 +0200 David Hildenbrand <[email protected]> wrote:

Let's introduce and use local error variables in the hotplug handler functions.

Signed-off-by David Hildenbrand <[email protected]>

---
 hw/s390x/s390-virtio-ccw.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

hw/s390x/s390-virtio-ccw.c +8 -3

index 7ae5fb38dd..29ea50a177 100644
@@ -434,18 +434,23 @@ static void s390_machine_reset(void)
 static void s390_machine_device_plug(HotplugHandler *hotplug_dev,
                                      DeviceState *dev, Error **errp)
 {
+    Error *local_err = NULL;
+
     if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
-        s390_cpu_plug(hotplug_dev, dev, errp);
+        s390_cpu_plug(hotplug_dev, dev, &local_err);
     }
+    error_propagate(errp, local_err);
 }
 
 static void s390_machine_device_unplug_request(HotplugHandler *hotplug_dev,
                                                DeviceState *dev, Error **errp)
 {
+    Error *local_err = NULL;
+
     if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
-        error_setg(errp, "CPU hot unplug not supported on this machine");
-        return;
+        error_setg(&local_err, "CPU hot unplug not supported on this machine");
     }
+    error_propagate(errp, local_err);
 }
 

Quoted 6 lines

hw/s390x/s390-virtio-ccw.c static void s390_machine_reset(void) · line 456

Quoted 1 line

hw/s390x/s390-virtio-ccw.c +0 -0

 static CpuInstanceProperties s390_cpu_index_to_props(MachineState *ms,

Just seeing this patch by itself, it does not really make much sense. Even if this is a split out clean-up series, I'd prefer this to go together with a patch that actually adds something more to the plug/unplug functions.

+1. It is hard to see the "why". Maybe a better patch description could help here?

Quoted 51 lines · Christian Borntraeger

Quoted 47 lines · Cornelia Huck

Quoted 29 lines · On Thu, 7 Jun 2018 18:52:18 +0200 David Hildenbrand <[email protected]> wrote:

Let's introduce and use local error variables in the hotplug handler functions.

Signed-off-by David Hildenbrand <[email protected]>

---
 hw/s390x/s390-virtio-ccw.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

hw/s390x/s390-virtio-ccw.c +5 -1

index 7ae5fb38dd..29ea50a177 100644
@@ -434,18 +434,23 @@ static void s390_machine_reset(void)
 static void s390_machine_device_plug(HotplugHandler *hotplug_dev,
                                      DeviceState *dev, Error **errp)
 {
+    Error *local_err = NULL;
+
     if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
-        s390_cpu_plug(hotplug_dev, dev, errp);
+        s390_cpu_plug(hotplug_dev, dev, &local_err);
     }
+    error_propagate(errp, local_err);
 }
 
 static void s390_machine_device_unplug_request(HotplugHandler *hotplug_dev,
                                                DeviceState *dev, Error **errp)
 {
+    Error *local_err = NULL;

hw/s390x/s390-virtio-ccw.c static void s390_machine_reset(void) · line 456

Quoted 10 lines

hw/s390x/s390-virtio-ccw.c +3 -2

+
     if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
-        error_setg(errp, "CPU hot unplug not supported on this machine");
-        return;
+        error_setg(&local_err, "CPU hot unplug not supported on this machine");
     }
+    error_propagate(errp, local_err);
 }
 
 static CpuInstanceProperties s390_cpu_index_to_props(MachineState *ms,

Just seeing this patch by itself, it does not really make much sense. Even if this is a split out clean-up series, I'd prefer this to go together with a patch that actually adds something more to the plug/unplug functions.

Quoted 2 lines

+1. It is hard to see the "why". Maybe a better patch description could help here?

When checking for an error (*errp) we should make sure that we don't dereference the NULL pointer. I will be doing that in the future (memory devices), but as you both don't seem to like this patch, I'll drop it for now.

Quoted 51 lines · Christian Borntraeger

Quoted 47 lines · Cornelia Huck

Quoted 29 lines · On Thu, 7 Jun 2018 18:52:18 +0200 David Hildenbrand <[email protected]> wrote:

Let's introduce and use local error variables in the hotplug handler functions.

Signed-off-by David Hildenbrand <[email protected]>

---
 hw/s390x/s390-virtio-ccw.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

hw/s390x/s390-virtio-ccw.c +5 -1

index 7ae5fb38dd..29ea50a177 100644
@@ -434,18 +434,23 @@ static void s390_machine_reset(void)
 static void s390_machine_device_plug(HotplugHandler *hotplug_dev,
                                      DeviceState *dev, Error **errp)
 {
+    Error *local_err = NULL;
+
     if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
-        s390_cpu_plug(hotplug_dev, dev, errp);
+        s390_cpu_plug(hotplug_dev, dev, &local_err);
     }
+    error_propagate(errp, local_err);
 }
 
 static void s390_machine_device_unplug_request(HotplugHandler *hotplug_dev,
                                                DeviceState *dev, Error **errp)
 {
+    Error *local_err = NULL;

hw/s390x/s390-virtio-ccw.c static void s390_machine_reset(void) · line 456

Quoted 10 lines

hw/s390x/s390-virtio-ccw.c +3 -2

+
     if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
-        error_setg(errp, "CPU hot unplug not supported on this machine");
-        return;
+        error_setg(&local_err, "CPU hot unplug not supported on this machine");
     }
+    error_propagate(errp, local_err);
 }
 
 static CpuInstanceProperties s390_cpu_index_to_props(MachineState *ms,  

Just seeing this patch by itself, it does not really make much sense. Even if this is a split out clean-up series, I'd prefer this to go together with a patch that actually adds something more to the plug/unplug functions.

Quoted 2 lines

+1. It is hard to see the "why". Maybe a better patch description could help here?

When checking for an error (*errp) we should make sure that we don't dereference the NULL pointer. I will be doing that in the future (memory devices), but as you both don't seem to like this patch, I'll drop it for now.

Quoted 55 lines · On Fri, 8 Jun 2018 09:40:04 +0200 David Hildenbrand <[email protected]> wrote:

Quoted 53 lines · Christian Borntraeger

Quoted 41 lines · Cornelia Huck

Quoted 38 lines · On Thu, 7 Jun 2018 18:52:18 +0200 David Hildenbrand <[email protected]> wrote:

Let's introduce and use local error variables in the hotplug handler functions.

Signed-off-by David Hildenbrand <[email protected]>

---
 hw/s390x/s390-virtio-ccw.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

hw/s390x/s390-virtio-ccw.c +8 -3

index 7ae5fb38dd..29ea50a177 100644
@@ -434,18 +434,23 @@ static void s390_machine_reset(void)
 static void s390_machine_device_plug(HotplugHandler *hotplug_dev,
                                      DeviceState *dev, Error **errp)
 {
+    Error *local_err = NULL;
+
     if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
-        s390_cpu_plug(hotplug_dev, dev, errp);
+        s390_cpu_plug(hotplug_dev, dev, &local_err);
     }
+    error_propagate(errp, local_err);
 }
 
 static void s390_machine_device_unplug_request(HotplugHandler *hotplug_dev,
                                                DeviceState *dev, Error **errp)
 {
+    Error *local_err = NULL;
+
     if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
-        error_setg(errp, "CPU hot unplug not supported on this machine");
-        return;
+        error_setg(&local_err, "CPU hot unplug not supported on this machine");
     }
+    error_propagate(errp, local_err);
 }
 

Quoted 6 lines

hw/s390x/s390-virtio-ccw.c static void s390_machine_reset(void) · line 456

Quoted 1 line

hw/s390x/s390-virtio-ccw.c +0 -0

 static CpuInstanceProperties s390_cpu_index_to_props(MachineState *ms,  

Just seeing this patch by itself, it does not really make much sense. Even if this is a split out clean-up series, I'd prefer this to go together with a patch that actually adds something more to the plug/unplug functions.

+1. It is hard to see the "why". Maybe a better patch description could help here?

Quoted 4 lines

When checking for an error (*errp) we should make sure that we don't dereference the NULL pointer. I will be doing that in the future (memory devices), but as you both don't seem to like this patch, I'll drop it for now.

hotplug handlers aren't called with NULL errp, so it not really necessary. To be on the safe side we can ensure that errp is not NULL doing something like:

hw/core/hotplug.c +1 -0

index 17ac986..dc9e4bf 100644
@@ -52,6 +52,7 @@ void hotplug_handler_unplug(HotplugHandler *plug_handler,
 {
     HotplugHandlerClass *hdc = HOTPLUG_HANDLER_GET_CLASS(plug_handler);
 
+    g_assert(errp);
     if (hdc->unplug) {
         hdc->unplug(plug_handler, plugged_dev, errp);
     }

and do it for all similar wrappers in this file

I'll be messing with machine hotplug handlers of pc/spapr/s390x in the context of
[PATCH v4 00/14] MemoryDevice: use multi stage hotplug handlers

So this is a spin-off of the cleanup patches produced so far.

David Hildenbrand (8):
pc: local error handling in hotplug handler functions
spapr: no need to verify the node
spapr: move all DIMM checks into spapr_memory_plug
spapr: local error handling in hotplug handler functions
spapr: introduce machine unplug handler
spapr: handle pc-dimm unplug via hotplug handler chain
spapr: handle cpu core unplug via hotplug handler chain
s390x: local error handling in hotplug handler functions

 hw/i386/pc.c               | 32 +++++++++-----
 hw/ppc/spapr.c             | 89 +++++++++++++++++++++++++-------------
 hw/s390x/s390-virtio-ccw.c | 11 +++--
 3 files changed, 89 insertions(+), 43 deletions(-)