When getting the 'pbdev', the if...else has no default branch.
From Coverity, the 'pbdev' maybe null when the 'dev' is not
the TYPE_PCI_BRIDGE/TYPE_PCI_DEVICE/TYPE_S390_PCI_DEVICE.
This patch adds a default branch for device plug and unplug.
Spotted by Coverity: CID 1398593
Signed-off-by: Li Qiang <liq3ea@163.com>
---
Adds a default branch for device plug per Cornelia's review.
hw/s390x/s390-pci-bus.c | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c
index 15759b6514..fe48a36ff6 100644
--- a/hw/s390x/s390-pci-bus.c
+++ b/hw/s390x/s390-pci-bus.c
@@ -912,6 +912,9 @@ static void s390_pcihost_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
pbdev->fh = pbdev->idx;
QTAILQ_INSERT_TAIL(&s->zpci_devs, pbdev, link);
g_hash_table_insert(s->zpci_table, &pbdev->idx, pbdev);
+ } else {
+ error_setg(errp, "s390: device plug request for not supported device"
+ " type: %s", object_get_typename(OBJECT(dev)));
}
}
@@ -956,6 +959,10 @@ static void s390_pcihost_unplug(HotplugHandler *hotplug_dev, DeviceState *dev,
} else if (object_dynamic_cast(OBJECT(dev), TYPE_S390_PCI_DEVICE)) {
pbdev = S390_PCI_DEVICE(dev);
pci_dev = pbdev->pdev;
+ } else {
+ error_setg(errp, "s390: device unplug request for not supported device"
+ " type: %s", object_get_typename(OBJECT(dev)));
+ return;
}
switch (pbdev->state) {
--
2.17.1
On Thu, 3 Jan 2019 07:16:12 -0800 Li Qiang <liq3ea@163.com> wrote: > When getting the 'pbdev', the if...else has no default branch. > From Coverity, the 'pbdev' maybe null when the 'dev' is not > the TYPE_PCI_BRIDGE/TYPE_PCI_DEVICE/TYPE_S390_PCI_DEVICE. > This patch adds a default branch for device plug and unplug. > > Spotted by Coverity: CID 1398593 > > Signed-off-by: Li Qiang <liq3ea@163.com> > --- > Adds a default branch for device plug per Cornelia's review. > > hw/s390x/s390-pci-bus.c | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c > index 15759b6514..fe48a36ff6 100644 > --- a/hw/s390x/s390-pci-bus.c > +++ b/hw/s390x/s390-pci-bus.c > @@ -912,6 +912,9 @@ static void s390_pcihost_plug(HotplugHandler *hotplug_dev, DeviceState *dev, > pbdev->fh = pbdev->idx; > QTAILQ_INSERT_TAIL(&s->zpci_devs, pbdev, link); > g_hash_table_insert(s->zpci_table, &pbdev->idx, pbdev); > + } else { > + error_setg(errp, "s390: device plug request for not supported device" > + " type: %s", object_get_typename(OBJECT(dev))); Maybe make this "s390/pci: plugging device type <%s> is not supported"? > } > } > > @@ -956,6 +959,10 @@ static void s390_pcihost_unplug(HotplugHandler *hotplug_dev, DeviceState *dev, > } else if (object_dynamic_cast(OBJECT(dev), TYPE_S390_PCI_DEVICE)) { > pbdev = S390_PCI_DEVICE(dev); > pci_dev = pbdev->pdev; > + } else { > + error_setg(errp, "s390: device unplug request for not supported device" > + " type: %s", object_get_typename(OBJECT(dev))); Halil has a point when he suggests an assert here. If we fence the device type in the plug function, I can't think of a way we would end up here. > + return; > } > > switch (pbdev->state) {
What do you think of ‘g_assert_not_reached();’. For example: else { g_assert_not_reached(); } Thanks, Li Qiang 发件人: Cornelia Huck 发送时间: 2019年1月4日 22:10 收件人: Li Qiang 抄送: walling@linux.ibm.com; rth@twiddle.net; david@redhat.com; pasic@linux.ibm.com; borntraeger@de.ibm.com; qemu-s390x@nongnu.org; qemu-devel@nongnu.org; peter.maydell@linaro.org 主题: Re: [PATCH v2] s390: avoid potential null dereference ins390_pcihost_unplug() On Thu, 3 Jan 2019 07:16:12 -0800 Li Qiang <liq3ea@163.com> wrote: > When getting the 'pbdev', the if...else has no default branch. > From Coverity, the 'pbdev' maybe null when the 'dev' is not > the TYPE_PCI_BRIDGE/TYPE_PCI_DEVICE/TYPE_S390_PCI_DEVICE. > This patch adds a default branch for device plug and unplug. > > Spotted by Coverity: CID 1398593 > > Signed-off-by: Li Qiang <liq3ea@163.com> > --- > Adds a default branch for device plug per Cornelia's review. > > hw/s390x/s390-pci-bus.c | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c > index 15759b6514..fe48a36ff6 100644 > --- a/hw/s390x/s390-pci-bus.c > +++ b/hw/s390x/s390-pci-bus.c > @@ -912,6 +912,9 @@ static void s390_pcihost_plug(HotplugHandler *hotplug_dev, DeviceState *dev, > pbdev->fh = pbdev->idx; > QTAILQ_INSERT_TAIL(&s->zpci_devs, pbdev, link); > g_hash_table_insert(s->zpci_table, &pbdev->idx, pbdev); > + } else { > + error_setg(errp, "s390: device plug request for not supported device" > + " type: %s", object_get_typename(OBJECT(dev))); Maybe make this "s390/pci: plugging device type <%s> is not supported"? > } > } > > @@ -956,6 +959,10 @@ static void s390_pcihost_unplug(HotplugHandler *hotplug_dev, DeviceState *dev, > } else if (object_dynamic_cast(OBJECT(dev), TYPE_S390_PCI_DEVICE)) { > pbdev = S390_PCI_DEVICE(dev); > pci_dev = pbdev->pdev; > + } else { > + error_setg(errp, "s390: device unplug request for not supported device" > + " type: %s", object_get_typename(OBJECT(dev))); Halil has a point when he suggests an assert here. If we fence the device type in the plug function, I can't think of a way we would end up here. > + return; > } > > switch (pbdev->state) {
On 04.01.19 15:33, Li Qiang wrote: > What do you think of ‘g_assert_not_reached();’. For example: > > > > else { > > g_assert_not_reached(); > > } > I agree, if thisever happens, it is a serious programming error, not an error to report to the user. (after all, he did nothing wrong) I'd prefer asserts. -- Thanks, David / dhildenb
On Fri, 4 Jan 2019 22:33:51 +0800 Li Qiang <liq3ea@163.com> wrote: > What do you think of ‘g_assert_not_reached();’. For example: > > else { > g_assert_not_reached(); > } Sounds good. But please return anyway in the unplug case, so that the code is fine if asserts have been configured out.
On Mon, 7 Jan 2019 at 15:48, Cornelia Huck <cohuck@redhat.com> wrote: > > On Fri, 4 Jan 2019 22:33:51 +0800 > Li Qiang <liq3ea@163.com> wrote: > > > What do you think of ‘g_assert_not_reached();’. For example: > > > > else { > > g_assert_not_reached(); > > } > > Sounds good. But please return anyway in the unplug case, so that the > code is fine if asserts have been configured out. Hopefully that won't cause the compiler to complain about unreachable code :-) thanks -- PMM
On Mon, 7 Jan 2019 15:54:21 +0000 Peter Maydell <peter.maydell@linaro.org> wrote: > On Mon, 7 Jan 2019 at 15:48, Cornelia Huck <cohuck@redhat.com> wrote: > > > > On Fri, 4 Jan 2019 22:33:51 +0800 > > Li Qiang <liq3ea@163.com> wrote: > > > > > What do you think of ‘g_assert_not_reached();’. For example: > > > > > > else { > > > g_assert_not_reached(); > > > } > > > > Sounds good. But please return anyway in the unplug case, so that the > > code is fine if asserts have been configured out. > > Hopefully that won't cause the compiler to complain about > unreachable code :-) BTW: Is there a common configuration where asserts are configured out? Not that this is an accident waiting to happen...
On Mon, 7 Jan 2019 at 15:57, Cornelia Huck <cohuck@redhat.com> wrote: > On Mon, 7 Jan 2019 15:54:21 +0000 > Peter Maydell <peter.maydell@linaro.org> wrote: > > On Mon, 7 Jan 2019 at 15:48, Cornelia Huck <cohuck@redhat.com> wrote: > > > Sounds good. But please return anyway in the unplug case, so that the > > > code is fine if asserts have been configured out. > > > > Hopefully that won't cause the compiler to complain about > > unreachable code :-) > > BTW: Is there a common configuration where asserts are configured out? > Not that this is an accident waiting to happen... No -- we insist they are always enabled, and osdep.h will #error out if either NDEBUG or G_DISABLE_ASSERT are set. thanks -- PMM
On Mon, 7 Jan 2019 16:04:35 +0000 Peter Maydell <peter.maydell@linaro.org> wrote: > On Mon, 7 Jan 2019 at 15:57, Cornelia Huck <cohuck@redhat.com> wrote: > > On Mon, 7 Jan 2019 15:54:21 +0000 > > Peter Maydell <peter.maydell@linaro.org> wrote: > > > On Mon, 7 Jan 2019 at 15:48, Cornelia Huck <cohuck@redhat.com> wrote: > > > > Sounds good. But please return anyway in the unplug case, so that the > > > > code is fine if asserts have been configured out. > > > > > > Hopefully that won't cause the compiler to complain about > > > unreachable code :-) > > > > BTW: Is there a common configuration where asserts are configured out? > > Not that this is an accident waiting to happen... > > No -- we insist they are always enabled, and osdep.h will #error > out if either NDEBUG or G_DISABLE_ASSERT are set. Ah, now I remember (I thought we still had that problem.) In that case, no return is needed.
At 2019-01-08 00:10:29, "Cornelia Huck" <cohuck@redhat.com> wrote: >On Mon, 7 Jan 2019 16:04:35 +0000 >Peter Maydell <peter.maydell@linaro.org> wrote: > >> On Mon, 7 Jan 2019 at 15:57, Cornelia Huck <cohuck@redhat.com> wrote: >> > On Mon, 7 Jan 2019 15:54:21 +0000 >> > Peter Maydell <peter.maydell@linaro.org> wrote: >> > > On Mon, 7 Jan 2019 at 15:48, Cornelia Huck <cohuck@redhat.com> wrote: >> > > > Sounds good. But please return anyway in the unplug case, so that the >> > > > code is fine if asserts have been configured out. >> > > >> > > Hopefully that won't cause the compiler to complain about >> > > unreachable code :-) >> > >> > BTW: Is there a common configuration where asserts are configured out? >> > Not that this is an accident waiting to happen... >> >> No -- we insist they are always enabled, and osdep.h will #error >> out if either NDEBUG or G_DISABLE_ASSERT are set. > >Ah, now I remember (I thought we still had that problem.) > >In that case, no return is needed. Ok, later I will send out a revised patch. Thanks, Li Qiang
On Fri, 4 Jan 2019 15:10:05 +0100 Cornelia Huck <cohuck@redhat.com> wrote: > On Thu, 3 Jan 2019 07:16:12 -0800 > Li Qiang <liq3ea@163.com> wrote: > > > When getting the 'pbdev', the if...else has no default branch. > > From Coverity, the 'pbdev' maybe null when the 'dev' is not > > the TYPE_PCI_BRIDGE/TYPE_PCI_DEVICE/TYPE_S390_PCI_DEVICE. > > This patch adds a default branch for device plug and unplug. > > > > Spotted by Coverity: CID 1398593 > > > > Signed-off-by: Li Qiang <liq3ea@163.com> > > --- > > Adds a default branch for device plug per Cornelia's review. > > > > hw/s390x/s390-pci-bus.c | 7 +++++++ > > 1 file changed, 7 insertions(+) > > > > diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c > > index 15759b6514..fe48a36ff6 100644 > > --- a/hw/s390x/s390-pci-bus.c > > +++ b/hw/s390x/s390-pci-bus.c > > @@ -912,6 +912,9 @@ static void s390_pcihost_plug(HotplugHandler *hotplug_dev, DeviceState *dev, > > pbdev->fh = pbdev->idx; > > QTAILQ_INSERT_TAIL(&s->zpci_devs, pbdev, link); > > g_hash_table_insert(s->zpci_table, &pbdev->idx, pbdev); > > + } else { > > + error_setg(errp, "s390: device plug request for not supported device" > > + " type: %s", object_get_typename(OBJECT(dev))); > > Maybe make this "s390/pci: plugging device type <%s> is not supported"? > Under what circumstances could/does this happen? I mean how can this be triggered by the user? Regards, Halil > > } > > } > > > > @@ -956,6 +959,10 @@ static void s390_pcihost_unplug(HotplugHandler *hotplug_dev, DeviceState *dev, > > } else if (object_dynamic_cast(OBJECT(dev), TYPE_S390_PCI_DEVICE)) { > > pbdev = S390_PCI_DEVICE(dev); > > pci_dev = pbdev->pdev; > > + } else { > > + error_setg(errp, "s390: device unplug request for not supported device" > > + " type: %s", object_get_typename(OBJECT(dev))); > > Halil has a point when he suggests an assert here. If we fence the > device type in the plug function, I can't think of a way we would end > up here. > > > + return; > > } > > > > switch (pbdev->state) { > >
On Fri, 4 Jan 2019 16:05:15 +0100 Halil Pasic <pasic@linux.ibm.com> wrote: > On Fri, 4 Jan 2019 15:10:05 +0100 > Cornelia Huck <cohuck@redhat.com> wrote: > > > On Thu, 3 Jan 2019 07:16:12 -0800 > > Li Qiang <liq3ea@163.com> wrote: > > > > > When getting the 'pbdev', the if...else has no default branch. > > > From Coverity, the 'pbdev' maybe null when the 'dev' is not > > > the TYPE_PCI_BRIDGE/TYPE_PCI_DEVICE/TYPE_S390_PCI_DEVICE. > > > This patch adds a default branch for device plug and unplug. > > > > > > Spotted by Coverity: CID 1398593 > > > > > > Signed-off-by: Li Qiang <liq3ea@163.com> > > > --- > > > Adds a default branch for device plug per Cornelia's review. > > > > > > hw/s390x/s390-pci-bus.c | 7 +++++++ > > > 1 file changed, 7 insertions(+) > > > > > > diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c > > > index 15759b6514..fe48a36ff6 100644 > > > --- a/hw/s390x/s390-pci-bus.c > > > +++ b/hw/s390x/s390-pci-bus.c > > > @@ -912,6 +912,9 @@ static void s390_pcihost_plug(HotplugHandler *hotplug_dev, DeviceState *dev, > > > pbdev->fh = pbdev->idx; > > > QTAILQ_INSERT_TAIL(&s->zpci_devs, pbdev, link); > > > g_hash_table_insert(s->zpci_table, &pbdev->idx, pbdev); > > > + } else { > > > + error_setg(errp, "s390: device plug request for not supported device" > > > + " type: %s", object_get_typename(OBJECT(dev))); > > > > Maybe make this "s390/pci: plugging device type <%s> is not supported"? > > > > Under what circumstances could/does this happen? I mean how can this > be triggered by the user? Probably only if a new type that can be plugged has been added, but the s390 pci code has not been updated. We could also assert, not sure what would make it easier to figure out what went wrong.
© 2016 - 2024 Red Hat, Inc.