[Qemu-devel] [PATCH 0/3] Refactor device_set_realized to avoid resource leak.

[Qemu-devel] [PATCH 0/3] Refactor device_set_realized to avoid resource leak.

[arei.gonglei]

From: Gonglei <arei.gonglei@huawei.com>

after committing
  [PATCH v6 0/9] Refactor PCI/SHPC/PCIE hotplug to use a more generic hotplug API

if devcie hotplgging failed, will casuse resource leak.

This patch series include address resouce leak and two other issuses.


BTW, for patch 2/3, checkpatch.py report a warning, but I have no idea how
to handle this probleam. Any ideas?

WARNING: line over 80 characters
#90: FILE: hw/core/qdev.c:866:
+                       dev->alias_required_for_version);

total: 0 errors, 1 warnings, 87 lines checked

Please review, thanks in advance.

Gonglei (3):
  qdev: add missing error check
  qdev: Refactor device_set_realized to avoid resource
  pcie: using error_setg instead of impolite assert

 hw/core/qdev.c | 80 +++++++++++++++++++++++++++++++++++++---------------------
 hw/pci/pcie.c  |  6 ++++-
 2 files changed, 56 insertions(+), 30 deletions(-)

-- 
1.7.12.4

[Qemu-devel] [PATCH 1/3] qdev: add missing error check

[arei.gonglei]

From: Gonglei <arei.gonglei@huawei.com>

If local_err is not null, the next code logic is useless.

Signed-off-by: Gonglei <arei.gonglei@huawei.com>
---
 hw/core/qdev.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index da1ba48..3e7085e 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -830,6 +830,11 @@ static void device_set_realized(Object *obj, bool value, Error **errp)
             g_free(name);
         }
 
+        if (local_err != NULL) {
+            error_propagate(errp, local_err);
+            return;
+        }
+
         if (dc->realize) {
             dc->realize(dev, &local_err);
         }
-- 
1.7.12.4

Re: [Qemu-devel] [PATCH 1/3] qdev: add missing error check

[Peter Crosthwaite]

On Tue, Aug 19, 2014 at 7:41 PM,  <arei.gonglei@huawei.com> wrote:
> From: Gonglei <arei.gonglei@huawei.com>
>
> If local_err is not null, the next code logic is useless.
>
> Signed-off-by: Gonglei <arei.gonglei@huawei.com>
> ---
>  hw/core/qdev.c | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> index da1ba48..3e7085e 100644
> --- a/hw/core/qdev.c
> +++ b/hw/core/qdev.c
> @@ -830,6 +830,11 @@ static void device_set_realized(Object *obj, bool value, Error **errp)
>              g_free(name);
>          }
>
> +        if (local_err != NULL) {
> +            error_propagate(errp, local_err);
> +            return;
> +        }
> +

So I'm curious to know if and how this manifested for you as a bug?
Can you reproduce this as a bug somehow even as a memory leak? as the
only way I can see local_err getting populated is a fail of:

            object_property_add_child(container_get(qdev_get_machine(),
                                                    "/unattached"),
                                      name, obj, &local_err);

Which, if fails indicates something very wrong. Should we promote that
fail to &error_abort and just drop the local_err logic entirely? I'm
very interested on your replication conditions on this one.

Regards,
Peter

>          if (dc->realize) {
>              dc->realize(dev, &local_err);
>          }
> --
> 1.7.12.4
>
>
>

Re: [Qemu-devel] [PATCH 1/3] qdev: add missing error check

[Paolo Bonzini]

Il 19/08/2014 15:50, Peter Crosthwaite ha scritto:
> So I'm curious to know if and how this manifested for you as a bug?
> Can you reproduce this as a bug somehow even as a memory leak? as the
> only way I can see local_err getting populated is a fail of:
> 
>             object_property_add_child(container_get(qdev_get_machine(),
>                                                     "/unattached"),
>                                       name, obj, &local_err);
> 
> Which, if fails indicates something very wrong. Should we promote that
> fail to &error_abort and just drop the local_err logic entirely? I'm
> very interested on your replication conditions on this one.

Yes, I don't think it can happen.  The user can only refer to
/machine/peripheral, not /machine/unattached.  &error_abort is better.

Paolo

Re: [Qemu-devel] [PATCH 1/3] qdev: add missing error check

[Gonglei (Arei)]

> -----Original Message-----
> From: Paolo Bonzini [mailto:pbonzini@redhat.com]
> Sent: Tuesday, August 19, 2014 10:03 PM
> To: Peter Crosthwaite; Gonglei (Arei)
> Cc: qemu-devel@nongnu.org Developers; Huangweidong (C); Michael S. Tsirkin;
> Huangpeng (Peter); Igor Mammedov; Andreas Färber
> Subject: Re: [Qemu-devel] [PATCH 1/3] qdev: add missing error check
> 
> Il 19/08/2014 15:50, Peter Crosthwaite ha scritto:
> > So I'm curious to know if and how this manifested for you as a bug?
> > Can you reproduce this as a bug somehow even as a memory leak? as the
> > only way I can see local_err getting populated is a fail of:
> >
> >
> object_property_add_child(container_get(qdev_get_machine(),
> >
> "/unattached"),
> >                                       name, obj, &local_err);
> >
> > Which, if fails indicates something very wrong. Should we promote that
> > fail to &error_abort and just drop the local_err logic entirely? I'm
> > very interested on your replication conditions on this one.
> 
> Yes, I don't think it can happen.  The user can only refer to
> /machine/peripheral, not /machine/unattached.  &error_abort is better.
> 
Yes. I agree, will do. 
Thanks, Paolo.

Best regards,
-Gonglei

Re: [Qemu-devel] [PATCH 1/3] qdev: add missing error check

[Gonglei (Arei)]

> > If local_err is not null, the next code logic is useless.
> >
> > Signed-off-by: Gonglei <arei.gonglei@huawei.com>
> > ---
> >  hw/core/qdev.c | 5 +++++
> >  1 file changed, 5 insertions(+)
> >
> > diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> > index da1ba48..3e7085e 100644
> > --- a/hw/core/qdev.c
> > +++ b/hw/core/qdev.c
> > @@ -830,6 +830,11 @@ static void device_set_realized(Object *obj, bool
> value, Error **errp)
> >              g_free(name);
> >          }
> >
> > +        if (local_err != NULL) {
> > +            error_propagate(errp, local_err);
> > +            return;
> > +        }
> > +
> 
> So I'm curious to know if and how this manifested for you as a bug?
> Can you reproduce this as a bug somehow even as a memory leak? as the
> only way I can see local_err getting populated is a fail of:
> 
>             object_property_add_child(container_get(qdev_get_machine(),
> 
> "/unattached"),
>                                       name, obj, &local_err);
> 
> Which, if fails indicates something very wrong. Should we promote that
> fail to &error_abort and just drop the local_err logic entirely? I'm
> very interested on your replication conditions on this one.
> 
Actually I just read code and find the problem. I can't agree more with you. 
Thanks, Peter!

Best regards,
-Gonglei

[Qemu-devel] [PATCH 2/3] qdev: Refactor device_set_realized to avoid resource leak

[arei.gonglei]

From: Gonglei <arei.gonglei@huawei.com>

At present, the local variable local_err is reused at multi-places,
Which will cause resource leak in some scenarios.

Example:

1. Assuming that "dc->realize(dev, &local_err)" execute successful
   and local_err == NULL;
2. Executing device hotplug in hotplug_handler_plug(), but failed
  (It is prone to occur). Then local_err != NULL;
3. error_propagate(errp, local_err) and return. But the resources
 which been allocated in dc->realize() will be leaked.
 Simple backtrace:
  dc->realize()
   |->device_realize
            |->pci_qdev_init()
                |->do_pci_register_device()
                |->etc.

To avoid the resource leak, using some different local error variables.
For different error conditions, release the corresponding resources.

Signed-off-by: Gonglei <arei.gonglei@huawei.com>
---
 hw/core/qdev.c | 75 +++++++++++++++++++++++++++++++++++-----------------------
 1 file changed, 46 insertions(+), 29 deletions(-)

diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index 3e7085e..b3a463b 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -839,41 +839,58 @@ static void device_set_realized(Object *obj, bool value, Error **errp)
             dc->realize(dev, &local_err);
         }
 
-        if (dev->parent_bus && dev->parent_bus->hotplug_handler &&
-            local_err == NULL) {
-            hotplug_handler_plug(dev->parent_bus->hotplug_handler,
-                                 dev, &local_err);
-        } else if (local_err == NULL &&
-                   object_dynamic_cast(qdev_get_machine(), TYPE_MACHINE)) {
-            HotplugHandler *hotplug_ctrl;
-            MachineState *machine = MACHINE(qdev_get_machine());
-            MachineClass *mc = MACHINE_GET_CLASS(machine);
-
-            if (mc->get_hotplug_handler) {
-                hotplug_ctrl = mc->get_hotplug_handler(machine, dev);
-                if (hotplug_ctrl) {
-                    hotplug_handler_plug(hotplug_ctrl, dev, &local_err);
+        if (local_err == NULL) {
+            Error *hotplug_err = NULL;
+
+            if (dev->parent_bus && dev->parent_bus->hotplug_handler) {
+                hotplug_handler_plug(dev->parent_bus->hotplug_handler,
+                                     dev, &hotplug_err);
+            } else if (object_dynamic_cast(qdev_get_machine(), TYPE_MACHINE)) {
+                HotplugHandler *hotplug_ctrl;
+                MachineState *machine = MACHINE(qdev_get_machine());
+                MachineClass *mc = MACHINE_GET_CLASS(machine);
+
+                if (mc->get_hotplug_handler) {
+                    hotplug_ctrl = mc->get_hotplug_handler(machine, dev);
+                    if (hotplug_ctrl) {
+                        hotplug_handler_plug(hotplug_ctrl, dev, &hotplug_err);
+                    }
                 }
             }
-        }
 
-        if (qdev_get_vmsd(dev) && local_err == NULL) {
-            vmstate_register_with_alias_id(dev, -1, qdev_get_vmsd(dev), dev,
-                                           dev->instance_id_alias,
-                                           dev->alias_required_for_version);
-        }
-        if (local_err == NULL) {
-            QLIST_FOREACH(bus, &dev->child_bus, sibling) {
-                object_property_set_bool(OBJECT(bus), true, "realized",
-                                         &local_err);
-                if (local_err != NULL) {
-                    break;
+            if (hotplug_err == NULL) {
+                Error *err = NULL;
+                if (qdev_get_vmsd(dev)) {
+                    vmstate_register_with_alias_id(dev, -1, qdev_get_vmsd(dev),
+                                                   dev, dev->instance_id_alias,
+                                                   dev->alias_required_for_version);
                 }
+
+                QLIST_FOREACH(bus, &dev->child_bus, sibling) {
+                    object_property_set_bool(OBJECT(bus), true, "realized",
+                                             &err);
+                    if (err != NULL) {
+                        if (qdev_get_vmsd(dev)) {
+                            vmstate_unregister(dev, qdev_get_vmsd(dev), dev);
+                        }
+
+                        break;
+                    }
+                }
+
+                if (dev->hotplugged && err == NULL) {
+                    device_reset(dev);
+                }
+                error_free(err);
+            } else {
+                if (dc->unrealize) {
+                    dc->unrealize(dev, NULL);
+                }
+
+                error_propagate(errp, hotplug_err);
+                return;
             }
         }
-        if (dev->hotplugged && local_err == NULL) {
-            device_reset(dev);
-        }
         dev->pending_deleted_event = false;
     } else if (!value && dev->realized) {
         QLIST_FOREACH(bus, &dev->child_bus, sibling) {
-- 
1.7.12.4

Re: [Qemu-devel] [PATCH 2/3] qdev: Refactor device_set_realized to avoid resource leak

[Peter Crosthwaite]

On Tue, Aug 19, 2014 at 7:41 PM,  <arei.gonglei@huawei.com> wrote:
> From: Gonglei <arei.gonglei@huawei.com>
>
> At present, the local variable local_err is reused at multi-places,
> Which will cause resource leak in some scenarios.
>

The problem isn't really the local_err reusage. It's the fact that
this function doesn't have partial cleanup implemented (the
dc->unrealize call you add here is needed but not in original code at
all). Doing a fuller audit of the function, it seems to have outgrown
the simplistic if (!local_err) approach to error handling. I think the
goto-fallthrough system might be a cleaner alternative. Perhaps finish
the fn with:

    dev->realized = value;
    return;

post_realize_fail:
    if (dc->unrealize) {
        dc->unrealize(dev, NULL);
    }
fail:
    if (local_err != NULL) {
        error_propagate(errp, local_err);
        return;
    }

}

Then goto the appropriate error label as local_err population is
detected as each relevant point.

Regards,
Peter

> Example:
>
> 1. Assuming that "dc->realize(dev, &local_err)" execute successful
>    and local_err == NULL;
> 2. Executing device hotplug in hotplug_handler_plug(), but failed
>   (It is prone to occur). Then local_err != NULL;
> 3. error_propagate(errp, local_err) and return. But the resources
>  which been allocated in dc->realize() will be leaked.
>  Simple backtrace:
>   dc->realize()
>    |->device_realize
>             |->pci_qdev_init()
>                 |->do_pci_register_device()
>                 |->etc.
>
> To avoid the resource leak, using some different local error variables.
> For different error conditions, release the corresponding resources.
>
> Signed-off-by: Gonglei <arei.gonglei@huawei.com>
> ---
>  hw/core/qdev.c | 75 +++++++++++++++++++++++++++++++++++-----------------------
>  1 file changed, 46 insertions(+), 29 deletions(-)
>
> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> index 3e7085e..b3a463b 100644
> --- a/hw/core/qdev.c
> +++ b/hw/core/qdev.c
> @@ -839,41 +839,58 @@ static void device_set_realized(Object *obj, bool value, Error **errp)
>              dc->realize(dev, &local_err);
>          }
>
> -        if (dev->parent_bus && dev->parent_bus->hotplug_handler &&
> -            local_err == NULL) {
> -            hotplug_handler_plug(dev->parent_bus->hotplug_handler,
> -                                 dev, &local_err);
> -        } else if (local_err == NULL &&
> -                   object_dynamic_cast(qdev_get_machine(), TYPE_MACHINE)) {
> -            HotplugHandler *hotplug_ctrl;
> -            MachineState *machine = MACHINE(qdev_get_machine());
> -            MachineClass *mc = MACHINE_GET_CLASS(machine);
> -
> -            if (mc->get_hotplug_handler) {
> -                hotplug_ctrl = mc->get_hotplug_handler(machine, dev);
> -                if (hotplug_ctrl) {
> -                    hotplug_handler_plug(hotplug_ctrl, dev, &local_err);
> +        if (local_err == NULL) {
> +            Error *hotplug_err = NULL;
> +
> +            if (dev->parent_bus && dev->parent_bus->hotplug_handler) {
> +                hotplug_handler_plug(dev->parent_bus->hotplug_handler,
> +                                     dev, &hotplug_err);
> +            } else if (object_dynamic_cast(qdev_get_machine(), TYPE_MACHINE)) {
> +                HotplugHandler *hotplug_ctrl;
> +                MachineState *machine = MACHINE(qdev_get_machine());
> +                MachineClass *mc = MACHINE_GET_CLASS(machine);
> +
> +                if (mc->get_hotplug_handler) {
> +                    hotplug_ctrl = mc->get_hotplug_handler(machine, dev);
> +                    if (hotplug_ctrl) {
> +                        hotplug_handler_plug(hotplug_ctrl, dev, &hotplug_err);
> +                    }
>                  }
>              }
> -        }
>
> -        if (qdev_get_vmsd(dev) && local_err == NULL) {
> -            vmstate_register_with_alias_id(dev, -1, qdev_get_vmsd(dev), dev,
> -                                           dev->instance_id_alias,
> -                                           dev->alias_required_for_version);
> -        }
> -        if (local_err == NULL) {
> -            QLIST_FOREACH(bus, &dev->child_bus, sibling) {
> -                object_property_set_bool(OBJECT(bus), true, "realized",
> -                                         &local_err);
> -                if (local_err != NULL) {
> -                    break;
> +            if (hotplug_err == NULL) {
> +                Error *err = NULL;
> +                if (qdev_get_vmsd(dev)) {
> +                    vmstate_register_with_alias_id(dev, -1, qdev_get_vmsd(dev),
> +                                                   dev, dev->instance_id_alias,
> +                                                   dev->alias_required_for_version);
>                  }
> +
> +                QLIST_FOREACH(bus, &dev->child_bus, sibling) {
> +                    object_property_set_bool(OBJECT(bus), true, "realized",
> +                                             &err);
> +                    if (err != NULL) {
> +                        if (qdev_get_vmsd(dev)) {
> +                            vmstate_unregister(dev, qdev_get_vmsd(dev), dev);
> +                        }
> +
> +                        break;
> +                    }
> +                }
> +
> +                if (dev->hotplugged && err == NULL) {
> +                    device_reset(dev);
> +                }
> +                error_free(err);
> +            } else {
> +                if (dc->unrealize) {
> +                    dc->unrealize(dev, NULL);
> +                }
> +
> +                error_propagate(errp, hotplug_err);
> +                return;
>              }
>          }
> -        if (dev->hotplugged && local_err == NULL) {
> -            device_reset(dev);
> -        }
>          dev->pending_deleted_event = false;
>      } else if (!value && dev->realized) {
>          QLIST_FOREACH(bus, &dev->child_bus, sibling) {
> --
> 1.7.12.4
>
>
>

Re: [Qemu-devel] [PATCH 2/3] qdev: Refactor device_set_realized to avoid resource leak

[Gonglei (Arei)]

> -----Original Message-----
> From: peter.crosthwaite@petalogix.com
> On Tue, Aug 19, 2014 at 7:41 PM,  <arei.gonglei@huawei.com> wrote:
> > From: Gonglei <arei.gonglei@huawei.com>
> >
> > At present, the local variable local_err is reused at multi-places,
> > Which will cause resource leak in some scenarios.
> >
> 
> The problem isn't really the local_err reusage. It's the fact that
> this function doesn't have partial cleanup implemented (the
> dc->unrealize call you add here is needed but not in original code at
> all). Doing a fuller audit of the function, it seems to have outgrown
> the simplistic if (!local_err) approach to error handling. I think the
> goto-fallthrough system might be a cleaner alternative. Perhaps finish
> the fn with:
> 
>     dev->realized = value;
>     return;
> 
> post_realize_fail:
>     if (dc->unrealize) {
>         dc->unrealize(dev, NULL);
>     }
> fail:
>     if (local_err != NULL) {
>         error_propagate(errp, local_err);
>         return;
>     }
> 
> }
> 
> Then goto the appropriate error label as local_err population is
> detected as each relevant point.
> 
Hi, Peter. I know your mean. vmstate_unregister() also should be called,
beside dc->unrealized(). 

I will re-realize as your suggestion. Thanks!

Best regards,
-Gonglei

[Qemu-devel] [PATCH 3/3] pcie: using error_setg instead of impolite assert

[arei.gonglei]

From: Gonglei <arei.gonglei@huawei.com>

It's enough of reporting an error. Assert() is not acceptable
because the error is not a fatal error.

Signed-off-by: Gonglei <arei.gonglei@huawei.com>
---
 hw/pci/pcie.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
index a123c01..7b46140 100644
--- a/hw/pci/pcie.c
+++ b/hw/pci/pcie.c
@@ -254,7 +254,11 @@ void pcie_cap_slot_hotplug_cb(HotplugHandler *hotplug_dev, DeviceState *dev,
      * Right now, only a device of function = 0 is allowed to be
      * hot plugged/unplugged.
      */
-    assert(PCI_FUNC(pci_dev->devfn) == 0);
+    if (PCI_FUNC(pci_dev->devfn) != 0) {
+        error_setg(errp, "Unsupported device function %d for PCIe hotplugging, "
+                   "only supported function 0", PCI_FUNC(pci_dev->devfn));
+        return;
+    }
 
     pci_word_test_and_set_mask(exp_cap + PCI_EXP_SLTSTA,
                                PCI_EXP_SLTSTA_PDS);
-- 
1.7.12.4

Re: [Qemu-devel] [PATCH 3/3] pcie: using error_setg instead of impolite assert

[Peter Crosthwaite]

Add "hotplug" to the subject line somewhere.


On Tue, Aug 19, 2014 at 7:41 PM,  <arei.gonglei@huawei.com> wrote:
> From: Gonglei <arei.gonglei@huawei.com>
>
> It's enough of reporting an error. Assert() is not acceptable

"It's enough to report an error".

Regards,
Peter

> because the error is not a fatal error.
>
> Signed-off-by: Gonglei <arei.gonglei@huawei.com>
> ---
>  hw/pci/pcie.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
> index a123c01..7b46140 100644
> --- a/hw/pci/pcie.c
> +++ b/hw/pci/pcie.c
> @@ -254,7 +254,11 @@ void pcie_cap_slot_hotplug_cb(HotplugHandler *hotplug_dev, DeviceState *dev,
>       * Right now, only a device of function = 0 is allowed to be
>       * hot plugged/unplugged.
>       */
> -    assert(PCI_FUNC(pci_dev->devfn) == 0);
> +    if (PCI_FUNC(pci_dev->devfn) != 0) {
> +        error_setg(errp, "Unsupported device function %d for PCIe hotplugging, "
> +                   "only supported function 0", PCI_FUNC(pci_dev->devfn));
> +        return;
> +    }
>
>      pci_word_test_and_set_mask(exp_cap + PCI_EXP_SLTSTA,
>                                 PCI_EXP_SLTSTA_PDS);
> --
> 1.7.12.4
>
>
>

Re: [Qemu-devel] [PATCH 3/3] pcie: using error_setg instead of impolite assert

[Gonglei (Arei)]

> -----Original Message-----
> From: peter.crosthwaite@petalogix.com
> [mailto:peter.crosthwaite@petalogix.com] On Behalf Of Peter Crosthwaite
> Sent: Tuesday, August 19, 2014 9:51 PM
> To: Gonglei (Arei)
> Cc: qemu-devel@nongnu.org Developers; Huangweidong (C); Michael S. Tsirkin;
> Huangpeng (Peter); Igor Mammedov; Paolo Bonzini; Andreas Färber
> Subject: Re: [Qemu-devel] [PATCH 3/3] pcie: using error_setg instead of
> impolite assert
> 
> Add "hotplug" to the subject line somewhere.
> 
OK.
> 
> On Tue, Aug 19, 2014 at 7:41 PM,  <arei.gonglei@huawei.com> wrote:
> > From: Gonglei <arei.gonglei@huawei.com>
> >
> > It's enough of reporting an error. Assert() is not acceptable
> 
> "It's enough to report an error".
> 
OK.

Thanks, Peter.

I'm waiting for MST's explicit comment for this patch.
If ok, will send v2, otherwise will drop it.

Best regards,
-Gonglei
 
> Regards,
> Peter
> 
> > because the error is not a fatal error.
> >
> > Signed-off-by: Gonglei <arei.gonglei@huawei.com>
> > ---
> >  hw/pci/pcie.c | 6 +++++-
> >  1 file changed, 5 insertions(+), 1 deletion(-)
> >
> > diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
> > index a123c01..7b46140 100644
> > --- a/hw/pci/pcie.c
> > +++ b/hw/pci/pcie.c
> > @@ -254,7 +254,11 @@ void pcie_cap_slot_hotplug_cb(HotplugHandler
> *hotplug_dev, DeviceState *dev,
> >       * Right now, only a device of function = 0 is allowed to be
> >       * hot plugged/unplugged.
> >       */
> > -    assert(PCI_FUNC(pci_dev->devfn) == 0);
> > +    if (PCI_FUNC(pci_dev->devfn) != 0) {
> > +        error_setg(errp, "Unsupported device function %d for PCIe
> hotplugging, "
> > +                   "only supported function 0",
> PCI_FUNC(pci_dev->devfn));
> > +        return;
> > +    }
> >
> >      pci_word_test_and_set_mask(exp_cap + PCI_EXP_SLTSTA,
> >                                 PCI_EXP_SLTSTA_PDS);
> > --
> > 1.7.12.4
> >
> >
> >

Re: [Qemu-devel] [PATCH 3/3] pcie: using error_setg instead of impolite assert

[Michael S. Tsirkin]

On Tue, Aug 19, 2014 at 05:41:45PM +0800, arei.gonglei@huawei.com wrote:
> From: Gonglei <arei.gonglei@huawei.com>
> 
> It's enough of reporting an error. Assert() is not acceptable
> because the error is not a fatal error.
> 
> Signed-off-by: Gonglei <arei.gonglei@huawei.com>
> ---
>  hw/pci/pcie.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
> index a123c01..7b46140 100644
> --- a/hw/pci/pcie.c
> +++ b/hw/pci/pcie.c
> @@ -254,7 +254,11 @@ void pcie_cap_slot_hotplug_cb(HotplugHandler *hotplug_dev, DeviceState *dev,
>       * Right now, only a device of function = 0 is allowed to be
>       * hot plugged/unplugged.
>       */
> -    assert(PCI_FUNC(pci_dev->devfn) == 0);
> +    if (PCI_FUNC(pci_dev->devfn) != 0) {
> +        error_setg(errp, "Unsupported device function %d for PCIe hotplugging, "
> +                   "only supported function 0", PCI_FUNC(pci_dev->devfn));
> +        return;
> +    }

Unplug of multifunction devices really should work.
Drop this assert and see what happens.

For hot-plug we don't have good APIs yet but we
really should define them rather than add more
code that we have to later remove.


>      pci_word_test_and_set_mask(exp_cap + PCI_EXP_SLTSTA,
>                                 PCI_EXP_SLTSTA_PDS);
> -- 
> 1.7.12.4
> 

Re: [Qemu-devel] [PATCH 3/3] pcie: using error_setg instead of impolite assert

[Gonglei (Arei)]

> > ---
> >  hw/pci/pcie.c | 6 +++++-
> >  1 file changed, 5 insertions(+), 1 deletion(-)
> >
> > diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
> > index a123c01..7b46140 100644
> > --- a/hw/pci/pcie.c
> > +++ b/hw/pci/pcie.c
> > @@ -254,7 +254,11 @@ void pcie_cap_slot_hotplug_cb(HotplugHandler
> *hotplug_dev, DeviceState *dev,
> >       * Right now, only a device of function = 0 is allowed to be
> >       * hot plugged/unplugged.
> >       */
> > -    assert(PCI_FUNC(pci_dev->devfn) == 0);
> > +    if (PCI_FUNC(pci_dev->devfn) != 0) {
> > +        error_setg(errp, "Unsupported device function %d for PCIe
> hotplugging, "
> > +                   "only supported function 0",
> PCI_FUNC(pci_dev->devfn));
> > +        return;
> > +    }
> 
> Unplug of multifunction devices really should work.
> Drop this assert and see what happens.
> 
I'm sorry to say that the device of function !=0 cannot been
recognized by guest os, which I have said in another conversation
[Why doesn't PCIe hotplug work for Q35 machine?]

> For hot-plug we don't have good APIs yet but we
> really should define them rather than add more
> code that we have to later remove.
> 
Hmm.. I just think the simple assert is not friendly
at present, if someone hotplug a device with function !0 .

MST, If you think this change is superfluous, I can drop this patch,
and I'm fine with it. Thanks.
 
Best regards,
-Gonglei