[PATCH-for-5.2 v2 0/2] block/block-backend: Let blk_attach_dev() provide helpful error

[PATCH-for-5.2 v2 0/2] block/block-backend: Let blk_attach_dev() provide helpful error

[Philippe Mathieu-Daudé]

A pair of patches which helps me debug an issue with block
drive already attached.

Suggestions to correctly/better use the Error API welcome, in
particular in qdev-properties-system::set_drive_helper().

Since v1:
- Rebased after 668f62ec62 ("error: Eliminate error_propagate()")

Philippe Mathieu-Daudé (2):
  block/block-backend: Trace blk_attach_dev()
  block/block-backend: Let blk_attach_dev() provide helpful error

 include/sysemu/block-backend.h   |  2 +-
 block/block-backend.c            | 12 +++++++++++-
 hw/block/fdc.c                   |  4 +---
 hw/block/swim.c                  |  4 +---
 hw/block/xen-block.c             |  5 +++--
 hw/core/qdev-properties-system.c | 16 +++++++++-------
 hw/ide/qdev.c                    |  4 +---
 hw/scsi/scsi-disk.c              |  4 +---
 block/trace-events               |  1 +
 9 files changed, 29 insertions(+), 23 deletions(-)

-- 
2.21.3

[PATCH-for-5.2 v2 1/2] block/block-backend: Trace blk_attach_dev()

[Philippe Mathieu-Daudé]

Add a trace event to follow devices attaching block drives:

  $ qemu-system-arm -M n800 -trace blk_\*
  9513@1594040428.738162:blk_attach_dev attaching blk 'sd0' to device 'omap2-mmc'
  9513@1594040428.738189:blk_attach_dev attaching blk 'sd0' to device 'sd-card'
  qemu-system-arm: sd_init failed: blk 'sd0' already attached by device 'sd-card'

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 block/block-backend.c | 1 +
 block/trace-events    | 1 +
 2 files changed, 2 insertions(+)

diff --git a/block/block-backend.c b/block/block-backend.c
index 0bf0188133..63ff940ef9 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -888,6 +888,7 @@ void blk_get_perm(BlockBackend *blk, uint64_t *perm, uint64_t *shared_perm)
  */
 int blk_attach_dev(BlockBackend *blk, DeviceState *dev)
 {
+    trace_blk_attach_dev(blk_name(blk), object_get_typename(OBJECT(dev)));
     if (blk->dev) {
         return -EBUSY;
     }
diff --git a/block/trace-events b/block/trace-events
index dbe76a7613..aa641c2034 100644
--- a/block/trace-events
+++ b/block/trace-events
@@ -9,6 +9,7 @@ blk_co_preadv(void *blk, void *bs, int64_t offset, unsigned int bytes, int flags
 blk_co_pwritev(void *blk, void *bs, int64_t offset, unsigned int bytes, int flags) "blk %p bs %p offset %"PRId64" bytes %u flags 0x%x"
 blk_root_attach(void *child, void *blk, void *bs) "child %p blk %p bs %p"
 blk_root_detach(void *child, void *blk, void *bs) "child %p blk %p bs %p"
+blk_attach_dev(const char *blk_name, const char *dev_name) "attaching blk '%s' to device '%s'"
 
 # io.c
 bdrv_co_preadv(void *bs, int64_t offset, int64_t nbytes, unsigned int flags) "bs %p offset %"PRId64" nbytes %"PRId64" flags 0x%x"
-- 
2.21.3

[RFC PATCH-for-5.2 v2 2/2] block/block-backend: Let blk_attach_dev() provide helpful error

[Philippe Mathieu-Daudé]

Let blk_attach_dev() take an Error* object to return helpful
information. Adapt the callers.

  $ qemu-system-arm -M n800
  qemu-system-arm: sd_init failed: cannot attach blk 'sd0' to device 'sd-card'
  blk 'sd0' is already attached by device 'omap2-mmc'
  Drive 'sd0' is already in use because it has been automatically connected to another device
  (do you need 'if=none' in the drive options?)

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
v2: Rebased after 668f62ec62 ("error: Eliminate error_propagate()")
---
 include/sysemu/block-backend.h   |  2 +-
 block/block-backend.c            | 11 ++++++++++-
 hw/block/fdc.c                   |  4 +---
 hw/block/swim.c                  |  4 +---
 hw/block/xen-block.c             |  5 +++--
 hw/core/qdev-properties-system.c | 16 +++++++++-------
 hw/ide/qdev.c                    |  4 +---
 hw/scsi/scsi-disk.c              |  4 +---
 8 files changed, 27 insertions(+), 23 deletions(-)

diff --git a/include/sysemu/block-backend.h b/include/sysemu/block-backend.h
index 8203d7f6f9..118fbad0b4 100644
--- a/include/sysemu/block-backend.h
+++ b/include/sysemu/block-backend.h
@@ -113,7 +113,7 @@ BlockDeviceIoStatus blk_iostatus(const BlockBackend *blk);
 void blk_iostatus_disable(BlockBackend *blk);
 void blk_iostatus_reset(BlockBackend *blk);
 void blk_iostatus_set_err(BlockBackend *blk, int error);
-int blk_attach_dev(BlockBackend *blk, DeviceState *dev);
+int blk_attach_dev(BlockBackend *blk, DeviceState *dev, Error **errp);
 void blk_detach_dev(BlockBackend *blk, DeviceState *dev);
 DeviceState *blk_get_attached_dev(BlockBackend *blk);
 char *blk_get_attached_dev_id(BlockBackend *blk);
diff --git a/block/block-backend.c b/block/block-backend.c
index 63ff940ef9..b7be0a4619 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -884,12 +884,21 @@ void blk_get_perm(BlockBackend *blk, uint64_t *perm, uint64_t *shared_perm)
 
 /*
  * Attach device model @dev to @blk.
+ *
+ * @blk: Block backend
+ * @dev: Device to attach the block backend to
+ * @errp: pointer to NULL initialized error object
+ *
  * Return 0 on success, -EBUSY when a device model is attached already.
  */
-int blk_attach_dev(BlockBackend *blk, DeviceState *dev)
+int blk_attach_dev(BlockBackend *blk, DeviceState *dev, Error **errp)
 {
     trace_blk_attach_dev(blk_name(blk), object_get_typename(OBJECT(dev)));
     if (blk->dev) {
+        error_setg(errp, "cannot attach blk '%s' to device '%s'",
+                   blk_name(blk), object_get_typename(OBJECT(dev)));
+        error_append_hint(errp, "blk '%s' is already attached by device '%s'\n",
+                          blk_name(blk), object_get_typename(OBJECT(blk->dev)));
         return -EBUSY;
     }
 
diff --git a/hw/block/fdc.c b/hw/block/fdc.c
index e9ed3eef45..bc39244152 100644
--- a/hw/block/fdc.c
+++ b/hw/block/fdc.c
@@ -519,7 +519,6 @@ static void floppy_drive_realize(DeviceState *qdev, Error **errp)
     FloppyBus *bus = FLOPPY_BUS(qdev->parent_bus);
     FDrive *drive;
     bool read_only;
-    int ret;
 
     if (dev->unit == -1) {
         for (dev->unit = 0; dev->unit < MAX_FD; dev->unit++) {
@@ -545,8 +544,7 @@ static void floppy_drive_realize(DeviceState *qdev, Error **errp)
     if (!dev->conf.blk) {
         /* Anonymous BlockBackend for an empty drive */
         dev->conf.blk = blk_new(qemu_get_aio_context(), 0, BLK_PERM_ALL);
-        ret = blk_attach_dev(dev->conf.blk, qdev);
-        assert(ret == 0);
+        blk_attach_dev(dev->conf.blk, qdev, &error_abort);
 
         /* Don't take write permissions on an empty drive to allow attaching a
          * read-only node later */
diff --git a/hw/block/swim.c b/hw/block/swim.c
index 74f56e8f46..2f1ecd0bb2 100644
--- a/hw/block/swim.c
+++ b/hw/block/swim.c
@@ -159,7 +159,6 @@ static void swim_drive_realize(DeviceState *qdev, Error **errp)
     SWIMDrive *dev = SWIM_DRIVE(qdev);
     SWIMBus *bus = SWIM_BUS(qdev->parent_bus);
     FDrive *drive;
-    int ret;
 
     if (dev->unit == -1) {
         for (dev->unit = 0; dev->unit < SWIM_MAX_FD; dev->unit++) {
@@ -185,8 +184,7 @@ static void swim_drive_realize(DeviceState *qdev, Error **errp)
     if (!dev->conf.blk) {
         /* Anonymous BlockBackend for an empty drive */
         dev->conf.blk = blk_new(qemu_get_aio_context(), 0, BLK_PERM_ALL);
-        ret = blk_attach_dev(dev->conf.blk, qdev);
-        assert(ret == 0);
+        blk_attach_dev(dev->conf.blk, qdev, &error_abort);
     }
 
     if (!blkconf_blocksizes(&dev->conf, errp)) {
diff --git a/hw/block/xen-block.c b/hw/block/xen-block.c
index 8a7a3f5452..eb99757faf 100644
--- a/hw/block/xen-block.c
+++ b/hw/block/xen-block.c
@@ -609,14 +609,15 @@ static void xen_cdrom_realize(XenBlockDevice *blockdev, Error **errp)
     blockdev->device_type = "cdrom";
 
     if (!conf->blk) {
+        Error *local_err = NULL;
         int rc;
 
         /* Set up an empty drive */
         conf->blk = blk_new(qemu_get_aio_context(), 0, BLK_PERM_ALL);
 
-        rc = blk_attach_dev(conf->blk, DEVICE(blockdev));
+        rc = blk_attach_dev(conf->blk, DEVICE(blockdev), &local_err);
         if (!rc) {
-            error_setg_errno(errp, -rc, "failed to create drive");
+            error_propagate_prepend(errp, local_err, "failed to create drive");
             return;
         }
     }
diff --git a/hw/core/qdev-properties-system.c b/hw/core/qdev-properties-system.c
index 3e4f16fc21..e3a757b1c3 100644
--- a/hw/core/qdev-properties-system.c
+++ b/hw/core/qdev-properties-system.c
@@ -136,17 +136,19 @@ static void set_drive_helper(Object *obj, Visitor *v, const char *name,
                    object_get_typename(OBJECT(dev)), prop->name, str);
         goto fail;
     }
-    if (blk_attach_dev(blk, dev) < 0) {
+    if (blk_attach_dev(blk, dev, errp) < 0) {
         DriveInfo *dinfo = blk_legacy_dinfo(blk);
 
         if (dinfo && dinfo->type != IF_NONE) {
-            error_setg(errp, "Drive '%s' is already in use because "
-                       "it has been automatically connected to another "
-                       "device (did you need 'if=none' in the drive options?)",
-                       str);
+            error_append_hint(errp,
+                              "Drive '%s' is already in use because it has "
+                              "been automatically connected to another device\n"
+                              "(do you need 'if=none' in the drive options?)\n",
+                              str);
         } else {
-            error_setg(errp, "Drive '%s' is already in use by another device",
-                       str);
+            error_append_hint(errp,
+                              "Drive '%s' is already in use by another device",
+                              str);
         }
         goto fail;
     }
diff --git a/hw/ide/qdev.c b/hw/ide/qdev.c
index 27ff1f7f66..9a02eb87f4 100644
--- a/hw/ide/qdev.c
+++ b/hw/ide/qdev.c
@@ -165,7 +165,6 @@ static void ide_dev_initfn(IDEDevice *dev, IDEDriveKind kind, Error **errp)
 {
     IDEBus *bus = DO_UPCAST(IDEBus, qbus, dev->qdev.parent_bus);
     IDEState *s = bus->ifs + dev->unit;
-    int ret;
 
     if (!dev->conf.blk) {
         if (kind != IDE_CD) {
@@ -174,8 +173,7 @@ static void ide_dev_initfn(IDEDevice *dev, IDEDriveKind kind, Error **errp)
         } else {
             /* Anonymous BlockBackend for an empty drive */
             dev->conf.blk = blk_new(qemu_get_aio_context(), 0, BLK_PERM_ALL);
-            ret = blk_attach_dev(dev->conf.blk, &dev->qdev);
-            assert(ret == 0);
+            blk_attach_dev(dev->conf.blk, &dev->qdev, &error_abort);
         }
     }
 
diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
index 8ce68a9dd6..92350642c7 100644
--- a/hw/scsi/scsi-disk.c
+++ b/hw/scsi/scsi-disk.c
@@ -2451,14 +2451,12 @@ static void scsi_cd_realize(SCSIDevice *dev, Error **errp)
 {
     SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, dev);
     AioContext *ctx;
-    int ret;
 
     if (!dev->conf.blk) {
         /* Anonymous BlockBackend for an empty drive. As we put it into
          * dev->conf, qdev takes care of detaching on unplug. */
         dev->conf.blk = blk_new(qemu_get_aio_context(), 0, BLK_PERM_ALL);
-        ret = blk_attach_dev(dev->conf.blk, &dev->qdev);
-        assert(ret == 0);
+        blk_attach_dev(dev->conf.blk, &dev->qdev, &error_abort);
     }
 
     ctx = blk_get_aio_context(dev->conf.blk);
-- 
2.21.3

Re: [PATCH-for-5.2 v2 1/2] block/block-backend: Trace blk_attach_dev()

[Philippe Mathieu-Daudé]

On 7/16/20 2:37 PM, Philippe Mathieu-Daudé wrote:
> Add a trace event to follow devices attaching block drives:
> 
>   $ qemu-system-arm -M n800 -trace blk_\*
>   9513@1594040428.738162:blk_attach_dev attaching blk 'sd0' to device 'omap2-mmc'
>   9513@1594040428.738189:blk_attach_dev attaching blk 'sd0' to device 'sd-card'
>   qemu-system-arm: sd_init failed: blk 'sd0' already attached by device 'sd-card'

Oops, the current error is:

    qemu-system-arm: sd_init failed: Drive 'sd0' is already in use by
another device

> 
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  block/block-backend.c | 1 +
>  block/trace-events    | 1 +
>  2 files changed, 2 insertions(+)
> 
> diff --git a/block/block-backend.c b/block/block-backend.c
> index 0bf0188133..63ff940ef9 100644
> --- a/block/block-backend.c
> +++ b/block/block-backend.c
> @@ -888,6 +888,7 @@ void blk_get_perm(BlockBackend *blk, uint64_t *perm, uint64_t *shared_perm)
>   */
>  int blk_attach_dev(BlockBackend *blk, DeviceState *dev)
>  {
> +    trace_blk_attach_dev(blk_name(blk), object_get_typename(OBJECT(dev)));
>      if (blk->dev) {
>          return -EBUSY;
>      }
> diff --git a/block/trace-events b/block/trace-events
> index dbe76a7613..aa641c2034 100644
> --- a/block/trace-events
> +++ b/block/trace-events
> @@ -9,6 +9,7 @@ blk_co_preadv(void *blk, void *bs, int64_t offset, unsigned int bytes, int flags
>  blk_co_pwritev(void *blk, void *bs, int64_t offset, unsigned int bytes, int flags) "blk %p bs %p offset %"PRId64" bytes %u flags 0x%x"
>  blk_root_attach(void *child, void *blk, void *bs) "child %p blk %p bs %p"
>  blk_root_detach(void *child, void *blk, void *bs) "child %p blk %p bs %p"
> +blk_attach_dev(const char *blk_name, const char *dev_name) "attaching blk '%s' to device '%s'"
>  
>  # io.c
>  bdrv_co_preadv(void *bs, int64_t offset, int64_t nbytes, unsigned int flags) "bs %p offset %"PRId64" nbytes %"PRId64" flags 0x%x"
> 

Re: [RFC PATCH-for-5.2 v2 2/2] block/block-backend: Let blk_attach_dev() provide helpful error

[Daniel P. Berrangé]

On Thu, Jul 16, 2020 at 02:37:04PM +0200, Philippe Mathieu-Daudé wrote:
> Let blk_attach_dev() take an Error* object to return helpful
> information. Adapt the callers.
> 
>   $ qemu-system-arm -M n800
>   qemu-system-arm: sd_init failed: cannot attach blk 'sd0' to device 'sd-card'
>   blk 'sd0' is already attached by device 'omap2-mmc'
>   Drive 'sd0' is already in use because it has been automatically connected to another device
>   (do you need 'if=none' in the drive options?)
> 
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
> v2: Rebased after 668f62ec62 ("error: Eliminate error_propagate()")
> ---
>  include/sysemu/block-backend.h   |  2 +-
>  block/block-backend.c            | 11 ++++++++++-
>  hw/block/fdc.c                   |  4 +---
>  hw/block/swim.c                  |  4 +---
>  hw/block/xen-block.c             |  5 +++--
>  hw/core/qdev-properties-system.c | 16 +++++++++-------
>  hw/ide/qdev.c                    |  4 +---
>  hw/scsi/scsi-disk.c              |  4 +---
>  8 files changed, 27 insertions(+), 23 deletions(-)
> 
> diff --git a/include/sysemu/block-backend.h b/include/sysemu/block-backend.h
> index 8203d7f6f9..118fbad0b4 100644
> --- a/include/sysemu/block-backend.h
> +++ b/include/sysemu/block-backend.h
> @@ -113,7 +113,7 @@ BlockDeviceIoStatus blk_iostatus(const BlockBackend *blk);
>  void blk_iostatus_disable(BlockBackend *blk);
>  void blk_iostatus_reset(BlockBackend *blk);
>  void blk_iostatus_set_err(BlockBackend *blk, int error);
> -int blk_attach_dev(BlockBackend *blk, DeviceState *dev);
> +int blk_attach_dev(BlockBackend *blk, DeviceState *dev, Error **errp);
>  void blk_detach_dev(BlockBackend *blk, DeviceState *dev);
>  DeviceState *blk_get_attached_dev(BlockBackend *blk);
>  char *blk_get_attached_dev_id(BlockBackend *blk);
> diff --git a/block/block-backend.c b/block/block-backend.c
> index 63ff940ef9..b7be0a4619 100644
> --- a/block/block-backend.c
> +++ b/block/block-backend.c
> @@ -884,12 +884,21 @@ void blk_get_perm(BlockBackend *blk, uint64_t *perm, uint64_t *shared_perm)
>  
>  /*
>   * Attach device model @dev to @blk.
> + *
> + * @blk: Block backend
> + * @dev: Device to attach the block backend to
> + * @errp: pointer to NULL initialized error object
> + *
>   * Return 0 on success, -EBUSY when a device model is attached already.
>   */
> -int blk_attach_dev(BlockBackend *blk, DeviceState *dev)
> +int blk_attach_dev(BlockBackend *blk, DeviceState *dev, Error **errp)
>  {
>      trace_blk_attach_dev(blk_name(blk), object_get_typename(OBJECT(dev)));
>      if (blk->dev) {
> +        error_setg(errp, "cannot attach blk '%s' to device '%s'",
> +                   blk_name(blk), object_get_typename(OBJECT(dev)));
> +        error_append_hint(errp, "blk '%s' is already attached by device '%s'\n",
> +                          blk_name(blk), object_get_typename(OBJECT(blk->dev)));

I would have a preference for expanding the main error message and not
using a hint.  Any hint is completely thrown away when using QMP :-(


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [RFC PATCH-for-5.2 v2 2/2] block/block-backend: Let blk_attach_dev() provide helpful error

[Markus Armbruster]

Daniel P. Berrangé <berrange@redhat.com> writes:

> On Thu, Jul 16, 2020 at 02:37:04PM +0200, Philippe Mathieu-Daudé wrote:
>> Let blk_attach_dev() take an Error* object to return helpful
>> information. Adapt the callers.
>> 
>>   $ qemu-system-arm -M n800
>>   qemu-system-arm: sd_init failed: cannot attach blk 'sd0' to device 'sd-card'
>>   blk 'sd0' is already attached by device 'omap2-mmc'
>>   Drive 'sd0' is already in use because it has been automatically connected to another device
>>   (do you need 'if=none' in the drive options?)
>> 
>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>> ---
>> v2: Rebased after 668f62ec62 ("error: Eliminate error_propagate()")
>> ---
>>  include/sysemu/block-backend.h   |  2 +-
>>  block/block-backend.c            | 11 ++++++++++-
>>  hw/block/fdc.c                   |  4 +---
>>  hw/block/swim.c                  |  4 +---
>>  hw/block/xen-block.c             |  5 +++--
>>  hw/core/qdev-properties-system.c | 16 +++++++++-------
>>  hw/ide/qdev.c                    |  4 +---
>>  hw/scsi/scsi-disk.c              |  4 +---
>>  8 files changed, 27 insertions(+), 23 deletions(-)
>> 
>> diff --git a/include/sysemu/block-backend.h b/include/sysemu/block-backend.h
>> index 8203d7f6f9..118fbad0b4 100644
>> --- a/include/sysemu/block-backend.h
>> +++ b/include/sysemu/block-backend.h
>> @@ -113,7 +113,7 @@ BlockDeviceIoStatus blk_iostatus(const BlockBackend *blk);
>>  void blk_iostatus_disable(BlockBackend *blk);
>>  void blk_iostatus_reset(BlockBackend *blk);
>>  void blk_iostatus_set_err(BlockBackend *blk, int error);
>> -int blk_attach_dev(BlockBackend *blk, DeviceState *dev);
>> +int blk_attach_dev(BlockBackend *blk, DeviceState *dev, Error **errp);
>>  void blk_detach_dev(BlockBackend *blk, DeviceState *dev);
>>  DeviceState *blk_get_attached_dev(BlockBackend *blk);
>>  char *blk_get_attached_dev_id(BlockBackend *blk);
>> diff --git a/block/block-backend.c b/block/block-backend.c
>> index 63ff940ef9..b7be0a4619 100644
>> --- a/block/block-backend.c
>> +++ b/block/block-backend.c
>> @@ -884,12 +884,21 @@ void blk_get_perm(BlockBackend *blk, uint64_t *perm, uint64_t *shared_perm)
>>  
>>  /*
>>   * Attach device model @dev to @blk.
>> + *
>> + * @blk: Block backend
>> + * @dev: Device to attach the block backend to
>> + * @errp: pointer to NULL initialized error object
>> + *
>>   * Return 0 on success, -EBUSY when a device model is attached already.
>>   */
>> -int blk_attach_dev(BlockBackend *blk, DeviceState *dev)
>> +int blk_attach_dev(BlockBackend *blk, DeviceState *dev, Error **errp)
>>  {
>>      trace_blk_attach_dev(blk_name(blk), object_get_typename(OBJECT(dev)));
>>      if (blk->dev) {
>> +        error_setg(errp, "cannot attach blk '%s' to device '%s'",
>> +                   blk_name(blk), object_get_typename(OBJECT(dev)));
>> +        error_append_hint(errp, "blk '%s' is already attached by device '%s'\n",
>> +                          blk_name(blk), object_get_typename(OBJECT(blk->dev)));
>
> I would have a preference for expanding the main error message and not
> using a hint.  Any hint is completely thrown away when using QMP :-(

Hints work best in cases like

    error message
    hint suggesting things to try to fix it, in CLI syntax

    error message rejecting a configuration value
    hint listing possible values, in CLI syntax

Why "in CLI syntax"?  Well, we need to use *some* syntax to be useful.
Hints have always been phrased for the CLI, simply because the hint
feature grew out of my disgust over the loss of lovingly written CLI
hints in the conversion to Error.

Hints in CLI syntax would be misleading QMP.  We never extended QMP to
transport hints.

Hints may tempt you in a case like

    error message that is painfully long, because it really tries hard to explain, which is laudable in theory, but sadly illegible in practice; also, interminable sentences, meh, this is an error message, not a Joyce novel

to get something like

    terse error message
    Explanation that is rather long, because it really tries hard to
    explain.  It can be multiple sentences, and lines are wrapped at a
    reasonable length.
    
Comes out okay in the CLI, but the explanation is lost in QMP.

I don't have a good solution to offer for errors that genuinely need
explaining.