Re: [Qemu-devel] [PATCH] virtio: abort on fatal error instead of just exiting

[Greg Kurz <groug@kaod.org>]

On Tue, 28 Jun 2016 10:24:29 +0200
Igor Mammedov <imammedo@redhat.com> wrote:

> replace mainly useless exit(1) on fatal error path with
> abort(), so that it would be possible to generate core
> dump, that could be used to analyse cause of problem.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---

Makes sense indeed.

Acked-by: Greg Kurz <groug@kaod.org>

FWIW, there's also a bunch of exit(1) in the device code:

$ git grep 'exit(1)' hw/virtio/ hw/*/virtio* hw/*/vhost*
hw/block/virtio-blk.c:        exit(1);
hw/block/virtio-blk.c:        exit(1);
hw/block/virtio-blk.c:        exit(1);
hw/net/virtio-net.c:            exit(1);
hw/net/virtio-net.c:            exit(1);
hw/net/virtio-net.c:            exit(1);
hw/net/virtio-net.c:            exit(1);
hw/net/virtio-net.c:                exit(1);
hw/scsi/vhost-scsi.c:        exit(1);
hw/scsi/vhost-scsi.c:            exit(1);
hw/scsi/virtio-scsi-dataplane.c:        exit(1);
hw/scsi/virtio-scsi.c:    exit(1);
hw/scsi/virtio-scsi.c:        exit(1);
hw/scsi/virtio-scsi.c:        exit(1);

>  hw/virtio/virtio.c | 24 ++++++++++++------------
>  1 file changed, 12 insertions(+), 12 deletions(-)
> 
> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> index 7ed06ea..9d3ac72 100644
> --- a/hw/virtio/virtio.c
> +++ b/hw/virtio/virtio.c
> @@ -315,7 +315,7 @@ static int virtqueue_num_heads(VirtQueue *vq, unsigned int idx)
>      if (num_heads > vq->vring.num) {
>          error_report("Guest moved used index from %u to %u",
>                       idx, vq->shadow_avail_idx);
> -        exit(1);
> +        abort();
>      }
>      /* On success, callers read a descriptor at vq->last_avail_idx.
>       * Make sure descriptor read does not bypass avail index read. */
> @@ -337,7 +337,7 @@ static unsigned int virtqueue_get_head(VirtQueue *vq, unsigned int idx)
>      /* If their number is silly, that's a fatal mistake. */
>      if (head >= vq->vring.num) {
>          error_report("Guest says index %u is available", head);
> -        exit(1);
> +        abort();
>      }
>  
>      return head;
> @@ -360,7 +360,7 @@ static unsigned virtqueue_read_next_desc(VirtIODevice *vdev, VRingDesc *desc,
>  
>      if (next >= max) {
>          error_report("Desc next is %u", next);
> -        exit(1);
> +        abort();
>      }
>  
>      vring_desc_read(vdev, desc, desc_pa, next);
> @@ -393,13 +393,13 @@ void virtqueue_get_avail_bytes(VirtQueue *vq, unsigned int *in_bytes,
>          if (desc.flags & VRING_DESC_F_INDIRECT) {
>              if (desc.len % sizeof(VRingDesc)) {
>                  error_report("Invalid size for indirect buffer table");
> -                exit(1);
> +                abort();
>              }
>  
>              /* If we've got too many, that implies a descriptor loop. */
>              if (num_bufs >= max) {
>                  error_report("Looped descriptor");
> -                exit(1);
> +                abort();
>              }
>  
>              /* loop over the indirect descriptor table */
> @@ -414,7 +414,7 @@ void virtqueue_get_avail_bytes(VirtQueue *vq, unsigned int *in_bytes,
>              /* If we've got too many, that implies a descriptor loop. */
>              if (++num_bufs > max) {
>                  error_report("Looped descriptor");
> -                exit(1);
> +                abort();
>              }
>  
>              if (desc.flags & VRING_DESC_F_WRITE) {
> @@ -462,7 +462,7 @@ static void virtqueue_map_desc(unsigned int *p_num_sg, hwaddr *addr, struct iove
>  
>          if (num_sg == max_num_sg) {
>              error_report("virtio: too many write descriptors in indirect table");
> -            exit(1);
> +            abort();
>          }
>  
>          iov[num_sg].iov_base = cpu_physical_memory_map(pa, &len, is_write);
> @@ -500,11 +500,11 @@ static void virtqueue_map_iovec(struct iovec *sg, hwaddr *addr,
>          sg[i].iov_base = cpu_physical_memory_map(addr[i], &len, is_write);
>          if (!sg[i].iov_base) {
>              error_report("virtio: error trying to map MMIO memory");
> -            exit(1);
> +            abort();
>          }
>          if (len != sg[i].iov_len) {
>              error_report("virtio: unexpected memory split");
> -            exit(1);
> +            abort();
>          }
>      }
>  }
> @@ -570,7 +570,7 @@ void *virtqueue_pop(VirtQueue *vq, size_t sz)
>      if (desc.flags & VRING_DESC_F_INDIRECT) {
>          if (desc.len % sizeof(VRingDesc)) {
>              error_report("Invalid size for indirect buffer table");
> -            exit(1);
> +            abort();
>          }
>  
>          /* loop over the indirect descriptor table */
> @@ -588,7 +588,7 @@ void *virtqueue_pop(VirtQueue *vq, size_t sz)
>          } else {
>              if (in_num) {
>                  error_report("Incorrect order for descriptors");
> -                exit(1);
> +                abort();
>              }
>              virtqueue_map_desc(&out_num, addr, iov,
>                                 VIRTQUEUE_MAX_SIZE, false, desc.addr, desc.len);
> @@ -597,7 +597,7 @@ void *virtqueue_pop(VirtQueue *vq, size_t sz)
>          /* If we've got too many, that implies a descriptor loop. */
>          if ((in_num + out_num) > max) {
>              error_report("Looped descriptor");
> -            exit(1);
> +            abort();
>          }
>      } while ((i = virtqueue_read_next_desc(vdev, &desc, desc_pa, max)) != max);
>