diff mbox

[qmp,v4,0/6] Add error_abort and associated cleanups

Message ID cover.1388630311.git.peter.crosthwaite@xilinx.com
State New
Headers show

Commit Message

Peter Crosthwaite Jan. 2, 2014, 2:46 a.m. UTC 
Following our discussion RE self asserting API calls, here is a spin of
my proposal. This series obsoletes the need for _nofail variants for
Error ** accepting APIs. Is also greatly reduces the verbosity of calls
sites that are currently asserting against errors.

Patch 1 is the main event - addition of error_abort. The following
patches then cleanup uses of _nofail and assert_no_error().

To give it a smoke test, I introduce a (critical) bug into QOM:

         prop->set(obj, v, prop->opaque, name, errp);

And run QEMU:

$ gdb --args ./microblazeel-softmmu/qemu-system-microblazeel -M petalogix-ml605 -nographic

Without application of this series, the bug manifests as:

qemu-system-microblazeel: Insufficient permission to perform this operation

Program received signal SIGABRT, Aborted.
(gdb) bt
0  0x00007ffff55f7425 in __GI_raise (sig=<optimized out>) at ../nptl/sysdeps/unix/sysv/linux/raise.c:64
1  0x00007ffff55fab8b in __GI_abort () at abort.c:91
2  0x00005555557073da in assert_no_error (err=<optimized out>) at qobject/qerror.c:128
3  0x0000555555615159 in qdev_property_add_static (dev=0x555555e9e6a0, prop=0x5555559ea4c0, errp=0x7fffffffe3c0) at hw/core/qdev.c:666
4  0x0000555555615355 in device_initfn (obj=<optimized out>) at hw/core/qdev.c:744

With this series, we now get a the full backtrace into the QOM API when
the assertion occurs (note stack frames 2-4 giving visibility into the
broken QOM API):

qemu-system-microblazeel: Insufficient permission to perform this operation

Program received signal SIGABRT, Aborted.
(gdb) bt
0  0x00007ffff55f7425 in __GI_raise (sig=<optimized out>) at ../nptl/sysdeps/unix/sysv/linux/raise.c:64
1  0x00007ffff55fab8b in __GI_abort () at abort.c:91
2  0x000055555570c5a4 in error_set (errp=0x555555e1b5f0, err_class=ERROR_CLASS_GENERIC_ERROR, fmt=0x55555571f5c0 "Insufficient permission to perform this operation") at util/error.c:47
3  0x00005555556778e5 in object_property_set_qobject (obj=0x555555e9e6a0, value=<optimized out>, name=0x55555574e7a6 "xlnx.base-vectors", errp=0x555555e1b5f0) at qom/qom-qobject.c:24
4  0x000055555567674d in object_property_set_int (obj=0x555555e9e6a0, value=<optimized out>, name=0x55555574e7a6 "xlnx.base-vectors", errp=0x555555e1b5f0) at qom/object.c:898
5  0x0000555555615192 in qdev_property_add_static (dev=0x555555e9e6a0, prop=0x5555559ea4c0, errp=0x555555e1b5f0) at hw/core/qdev.c:664
6  0x000055555561534f in device_initfn (obj=<optimized out>) at hw/core/qdev.c:741

Changed since v2:
Removed new assert_no_error usages in target-arm/cpu.c
Changed since v1:
Addressed Markus review.
Guarded against erroneous setting of error_abort != NULL.


Peter Crosthwaite (6):
  error: Add error_abort
  hw/core/qdev: Delete dead code
  hw: Remove assert_no_error usages
  target-i386: Remove assert_no_error usage
  qemu-option: Remove qemu_opts_create_nofail
  qerror: Remove assert_no_error()

 block/blkdebug.c                 |  2 +-
 block/blkverify.c                |  2 +-
 block/curl.c                     |  2 +-
 block/gluster.c                  |  2 +-
 block/iscsi.c                    |  2 +-
 block/nbd.c                      |  3 ++-
 block/qcow2.c                    |  2 +-
 block/raw-posix.c                |  2 +-
 block/raw-win32.c                |  5 +++--
 block/rbd.c                      |  2 +-
 block/sheepdog.c                 |  2 +-
 block/vvfat.c                    |  2 +-
 blockdev.c                       |  6 ++++--
 hw/core/qdev-properties-system.c |  8 ++------
 hw/core/qdev-properties.c        | 40 ++++++++++------------------------------
 hw/core/qdev.c                   | 28 +++++++---------------------
 hw/dma/xilinx_axidma.c           | 13 ++++---------
 hw/net/xilinx_axienet.c          | 13 ++++---------
 hw/watchdog/watchdog.c           |  3 ++-
 include/hw/xilinx.h              | 14 ++++----------
 include/qapi/error.h             |  6 ++++++
 include/qapi/qmp/qerror.h        |  1 -
 include/qemu/option.h            |  1 -
 qdev-monitor.c                   |  2 +-
 qemu-img.c                       |  2 +-
 qobject/qerror.c                 |  8 --------
 target-arm/cpu.c                 |  7 ++-----
 target-i386/cpu.c                |  4 +---
 util/error.c                     | 22 +++++++++++++++++++++-
 util/qemu-config.c               |  2 +-
 util/qemu-option.c               |  9 ---------
 util/qemu-sockets.c              | 18 +++++++++---------
 vl.c                             | 15 +++++++++------
 33 files changed, 103 insertions(+), 147 deletions(-)

Comments

Luiz Capitulino Jan. 6, 2014, 7:13 p.m. UTC | #1 
On Wed,  1 Jan 2014 18:46:24 -0800
Peter Crosthwaite <peter.crosthwaite@xilinx.com> wrote:

> Following our discussion RE self asserting API calls, here is a spin of
> my proposal. This series obsoletes the need for _nofail variants for
> Error ** accepting APIs. Is also greatly reduces the verbosity of calls
> sites that are currently asserting against errors.
> 
> Patch 1 is the main event - addition of error_abort. The following
> patches then cleanup uses of _nofail and assert_no_error().

Applied to the qmp branch, thanks.

> 
> To give it a smoke test, I introduce a (critical) bug into QOM:
> 
> --- a/qom/object.c
> +++ b/qom/object.c
> @@ -797,7 +797,7 @@ void object_property_set(Object *obj, Visitor *v,
> const char *name,
>          return;
>      }
> 
> -    if (!prop->set) {
> +    if (prop->set) {
>          error_set(errp, QERR_PERMISSION_DENIED);
>      } else {
>          prop->set(obj, v, prop->opaque, name, errp);
> 
> And run QEMU:
> 
> $ gdb --args ./microblazeel-softmmu/qemu-system-microblazeel -M petalogix-ml605 -nographic
> 
> Without application of this series, the bug manifests as:
> 
> qemu-system-microblazeel: Insufficient permission to perform this operation
> 
> Program received signal SIGABRT, Aborted.
> (gdb) bt
> 0  0x00007ffff55f7425 in __GI_raise (sig=<optimized out>) at ../nptl/sysdeps/unix/sysv/linux/raise.c:64
> 1  0x00007ffff55fab8b in __GI_abort () at abort.c:91
> 2  0x00005555557073da in assert_no_error (err=<optimized out>) at qobject/qerror.c:128
> 3  0x0000555555615159 in qdev_property_add_static (dev=0x555555e9e6a0, prop=0x5555559ea4c0, errp=0x7fffffffe3c0) at hw/core/qdev.c:666
> 4  0x0000555555615355 in device_initfn (obj=<optimized out>) at hw/core/qdev.c:744
> 
> With this series, we now get a the full backtrace into the QOM API when
> the assertion occurs (note stack frames 2-4 giving visibility into the
> broken QOM API):
> 
> qemu-system-microblazeel: Insufficient permission to perform this operation
> 
> Program received signal SIGABRT, Aborted.
> (gdb) bt
> 0  0x00007ffff55f7425 in __GI_raise (sig=<optimized out>) at ../nptl/sysdeps/unix/sysv/linux/raise.c:64
> 1  0x00007ffff55fab8b in __GI_abort () at abort.c:91
> 2  0x000055555570c5a4 in error_set (errp=0x555555e1b5f0, err_class=ERROR_CLASS_GENERIC_ERROR, fmt=0x55555571f5c0 "Insufficient permission to perform this operation") at util/error.c:47
> 3  0x00005555556778e5 in object_property_set_qobject (obj=0x555555e9e6a0, value=<optimized out>, name=0x55555574e7a6 "xlnx.base-vectors", errp=0x555555e1b5f0) at qom/qom-qobject.c:24
> 4  0x000055555567674d in object_property_set_int (obj=0x555555e9e6a0, value=<optimized out>, name=0x55555574e7a6 "xlnx.base-vectors", errp=0x555555e1b5f0) at qom/object.c:898
> 5  0x0000555555615192 in qdev_property_add_static (dev=0x555555e9e6a0, prop=0x5555559ea4c0, errp=0x555555e1b5f0) at hw/core/qdev.c:664
> 6  0x000055555561534f in device_initfn (obj=<optimized out>) at hw/core/qdev.c:741
> 
> Changed since v2:
> Removed new assert_no_error usages in target-arm/cpu.c
> Changed since v1:
> Addressed Markus review.
> Guarded against erroneous setting of error_abort != NULL.
> 
> 
> Peter Crosthwaite (6):
>   error: Add error_abort
>   hw/core/qdev: Delete dead code
>   hw: Remove assert_no_error usages
>   target-i386: Remove assert_no_error usage
>   qemu-option: Remove qemu_opts_create_nofail
>   qerror: Remove assert_no_error()
> 
>  block/blkdebug.c                 |  2 +-
>  block/blkverify.c                |  2 +-
>  block/curl.c                     |  2 +-
>  block/gluster.c                  |  2 +-
>  block/iscsi.c                    |  2 +-
>  block/nbd.c                      |  3 ++-
>  block/qcow2.c                    |  2 +-
>  block/raw-posix.c                |  2 +-
>  block/raw-win32.c                |  5 +++--
>  block/rbd.c                      |  2 +-
>  block/sheepdog.c                 |  2 +-
>  block/vvfat.c                    |  2 +-
>  blockdev.c                       |  6 ++++--
>  hw/core/qdev-properties-system.c |  8 ++------
>  hw/core/qdev-properties.c        | 40 ++++++++++------------------------------
>  hw/core/qdev.c                   | 28 +++++++---------------------
>  hw/dma/xilinx_axidma.c           | 13 ++++---------
>  hw/net/xilinx_axienet.c          | 13 ++++---------
>  hw/watchdog/watchdog.c           |  3 ++-
>  include/hw/xilinx.h              | 14 ++++----------
>  include/qapi/error.h             |  6 ++++++
>  include/qapi/qmp/qerror.h        |  1 -
>  include/qemu/option.h            |  1 -
>  qdev-monitor.c                   |  2 +-
>  qemu-img.c                       |  2 +-
>  qobject/qerror.c                 |  8 --------
>  target-arm/cpu.c                 |  7 ++-----
>  target-i386/cpu.c                |  4 +---
>  util/error.c                     | 22 +++++++++++++++++++++-
>  util/qemu-config.c               |  2 +-
>  util/qemu-option.c               |  9 ---------
>  util/qemu-sockets.c              | 18 +++++++++---------
>  vl.c                             | 15 +++++++++------
>  33 files changed, 103 insertions(+), 147 deletions(-)
>
Andreas Färber Jan. 6, 2014, 7:18 p.m. UTC | #2 
Am 06.01.2014 20:13, schrieb Luiz Capitulino:
> On Wed,  1 Jan 2014 18:46:24 -0800
> Peter Crosthwaite <peter.crosthwaite@xilinx.com> wrote:
> 
>> Following our discussion RE self asserting API calls, here is a spin of
>> my proposal. This series obsoletes the need for _nofail variants for
>> Error ** accepting APIs. Is also greatly reduces the verbosity of calls
>> sites that are currently asserting against errors.
>>
>> Patch 1 is the main event - addition of error_abort. The following
>> patches then cleanup uses of _nofail and assert_no_error().
> 
> Applied to the qmp branch, thanks.

Please normalize "hw/core/qdev:" to just "qdev:", thanks.

Andreas

>> Peter Crosthwaite (6):
>>   error: Add error_abort
>>   hw/core/qdev: Delete dead code
>>   hw: Remove assert_no_error usages
>>   target-i386: Remove assert_no_error usage
>>   qemu-option: Remove qemu_opts_create_nofail
>>   qerror: Remove assert_no_error()
Luiz Capitulino Jan. 6, 2014, 7:26 p.m. UTC | #3 
On Mon, 06 Jan 2014 20:18:45 +0100
Andreas Färber <afaerber@suse.de> wrote:

> Am 06.01.2014 20:13, schrieb Luiz Capitulino:
> > On Wed,  1 Jan 2014 18:46:24 -0800
> > Peter Crosthwaite <peter.crosthwaite@xilinx.com> wrote:
> > 
> >> Following our discussion RE self asserting API calls, here is a spin of
> >> my proposal. This series obsoletes the need for _nofail variants for
> >> Error ** accepting APIs. Is also greatly reduces the verbosity of calls
> >> sites that are currently asserting against errors.
> >>
> >> Patch 1 is the main event - addition of error_abort. The following
> >> patches then cleanup uses of _nofail and assert_no_error().
> > 
> > Applied to the qmp branch, thanks.
> 
> Please normalize "hw/core/qdev:" to just "qdev:", thanks.

Done. You still have time for a Reviewed-by :)
diff mbox

Patch

--- a/qom/object.c
+++ b/qom/object.c
@@ -797,7 +797,7 @@  void object_property_set(Object *obj, Visitor *v,
const char *name,
         return;
     }

-    if (!prop->set) {
+    if (prop->set) {
         error_set(errp, QERR_PERMISSION_DENIED);
     } else {