| Message ID | 20170623164557.11636-4-f4bug@amsat.org |
|---|---|
| State | New |
| Headers | show |
On Fri, Jun 23, 2017 at 01:45:57PM -0300, Philippe Mathieu-Daudé wrote: > then abort calling error_setg() I don't understand the reasons for this. This commit message says "what" and "how", but not "why". > > Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> > --- > hw/core/qdev.c | 6 +++++- > 1 file changed, 5 insertions(+), 1 deletion(-) > > diff --git a/hw/core/qdev.c b/hw/core/qdev.c > index 849952a8d4..05aaa67cb8 100644 > --- a/hw/core/qdev.c > +++ b/hw/core/qdev.c > @@ -448,7 +448,11 @@ qemu_irq qdev_get_gpio_in_named(DeviceState *dev, const char *name, int n) > { > NamedGPIOList *gpio_list = qdev_get_named_gpio_list(dev, name); > > - assert(n >= 0 && n < gpio_list->num_in); > + assert(n >= 0); > + if (n >= gpio_list->num_in) { > + error_setg(&error_abort, "Invalid gpio #%d (of %d) for %s", > + n, gpio_list->num_in, name ? name : "device"); Why exactly assert() is ok for (n < 0), but not for (n >= gpio_list->num_io)? If you have reasons to believe (n >= gpio_list->num_in) can be triggered by user input, then abort() isn't an appropriate way to handle it. > + } > return gpio_list->in[n]; > } > > -- > 2.13.1 >
On Fri, Jun 23, 2017 at 01:45:57PM -0300, Philippe Mathieu-Daudé wrote: > then abort calling error_setg() > > Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> > --- > hw/core/qdev.c | 6 +++++- > 1 file changed, 5 insertions(+), 1 deletion(-) > > diff --git a/hw/core/qdev.c b/hw/core/qdev.c > index 849952a8d4..05aaa67cb8 100644 > --- a/hw/core/qdev.c > +++ b/hw/core/qdev.c > @@ -448,7 +448,11 @@ qemu_irq qdev_get_gpio_in_named(DeviceState *dev, const char *name, int n) > { > NamedGPIOList *gpio_list = qdev_get_named_gpio_list(dev, name); > > - assert(n >= 0 && n < gpio_list->num_in); > + assert(n >= 0); > + if (n >= gpio_list->num_in) { > + error_setg(&error_abort, "Invalid gpio #%d (of %d) for %s", > + n, gpio_list->num_in, name ? name : "device"); I just noticed error_setg() documentation explicitly discourages this. """ Please don't error_setg(&error_fatal, ...), use error_report() and exit(), because that's more obvious. Likewise, don't error_setg(&error_abort, ...), use assert(). """
On Fri, Jun 23, 2017 at 9:45 AM, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote: > then abort calling error_setg() The commit message should be able to be read separately from the title. Once you fix up the message: Reviewed-by: Alistair Francis <alistair.francis@xilinx.com> Thanks, Alistair > > Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> > --- > hw/core/qdev.c | 6 +++++- > 1 file changed, 5 insertions(+), 1 deletion(-) > > diff --git a/hw/core/qdev.c b/hw/core/qdev.c > index 849952a8d4..05aaa67cb8 100644 > --- a/hw/core/qdev.c > +++ b/hw/core/qdev.c > @@ -448,7 +448,11 @@ qemu_irq qdev_get_gpio_in_named(DeviceState *dev, const char *name, int n) > { > NamedGPIOList *gpio_list = qdev_get_named_gpio_list(dev, name); > > - assert(n >= 0 && n < gpio_list->num_in); > + assert(n >= 0); > + if (n >= gpio_list->num_in) { > + error_setg(&error_abort, "Invalid gpio #%d (of %d) for %s", > + n, gpio_list->num_in, name ? name : "device"); > + } > return gpio_list->in[n]; > } > > -- > 2.13.1 > >
On 06/23/2017 02:25 PM, Eduardo Habkost wrote: > On Fri, Jun 23, 2017 at 01:45:57PM -0300, Philippe Mathieu-Daudé wrote: >> then abort calling error_setg() > > I don't understand the reasons for this. This commit message says > "what" and "how", but not "why". > >> - assert(n >= 0 && n < gpio_list->num_in); >> + assert(n >= 0); >> + if (n >= gpio_list->num_in) { >> + error_setg(&error_abort, "Invalid gpio #%d (of %d) for %s", >> + n, gpio_list->num_in, name ? name : "device"); > > Why exactly assert() is ok for (n < 0), but not for > (n >= gpio_list->num_io)? > > If you have reasons to believe (n >= gpio_list->num_in) can be triggered > by user input, then abort() isn't an appropriate way to handle it. What's more, error_setg(&error_abort) should not be used. Yes, a quick grep for 'error_setg.*error_abort' shows that we have a couple of bad examples in the tree that should be patched, but it also finds that include/qapi/error.h states: * Please don't error_setg(&error_fatal, ...), use error_report() and * exit(), because that's more obvious. * Likewise, don't error_setg(&error_abort, ...), use assert().
diff --git a/hw/core/qdev.c b/hw/core/qdev.c index 849952a8d4..05aaa67cb8 100644 --- a/hw/core/qdev.c +++ b/hw/core/qdev.c @@ -448,7 +448,11 @@ qemu_irq qdev_get_gpio_in_named(DeviceState *dev, const char *name, int n) { NamedGPIOList *gpio_list = qdev_get_named_gpio_list(dev, name); - assert(n >= 0 && n < gpio_list->num_in); + assert(n >= 0); + if (n >= gpio_list->num_in) { + error_setg(&error_abort, "Invalid gpio #%d (of %d) for %s", + n, gpio_list->num_in, name ? name : "device"); + } return gpio_list->in[n]; }
then abort calling error_setg() Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> --- hw/core/qdev.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)