* Re: [Qemu-devel] [PATCH] virtio: abort on fatal error instead of just exiting
2016-06-28 8:24 [Qemu-devel] [PATCH] virtio: abort on fatal error instead of just exiting Igor Mammedov
@ 2016-06-28 9:05 ` Greg Kurz
2016-06-28 10:42 ` Cornelia Huck
` (2 subsequent siblings)
3 siblings, 0 replies; 8+ messages in thread
From: Greg Kurz @ 2016-06-28 9:05 UTC (permalink / raw)
To: Igor Mammedov; +Cc: qemu-devel, mst
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);
>
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [Qemu-devel] [PATCH] virtio: abort on fatal error instead of just exiting
2016-06-28 8:24 [Qemu-devel] [PATCH] virtio: abort on fatal error instead of just exiting Igor Mammedov
2016-06-28 9:05 ` Greg Kurz
@ 2016-06-28 10:42 ` Cornelia Huck
2016-06-29 12:49 ` Markus Armbruster
2016-07-27 18:13 ` Michael S. Tsirkin
3 siblings, 0 replies; 8+ messages in thread
From: Cornelia Huck @ 2016-06-28 10:42 UTC (permalink / raw)
To: Igor Mammedov; +Cc: qemu-devel, mst
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>
> ---
> hw/virtio/virtio.c | 24 ++++++++++++------------
> 1 file changed, 12 insertions(+), 12 deletions(-)
There had been https://marc.info/?l=qemu-devel&m=146046754901035&w=2
which instead marks devices as broken; but I don't think there has been
progress since then.
Switching to abort() might be a stop-gap solution so we can at least
analyze what went bad, so
Acked-by: Cornelia Huck <cornelia.huck@de.ibm.com>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH] virtio: abort on fatal error instead of just exiting
2016-06-28 8:24 [Qemu-devel] [PATCH] virtio: abort on fatal error instead of just exiting Igor Mammedov
2016-06-28 9:05 ` Greg Kurz
2016-06-28 10:42 ` Cornelia Huck
@ 2016-06-29 12:49 ` Markus Armbruster
2016-06-29 16:36 ` Igor Mammedov
2016-07-27 18:13 ` Michael S. Tsirkin
3 siblings, 1 reply; 8+ messages in thread
From: Markus Armbruster @ 2016-06-29 12:49 UTC (permalink / raw)
To: Igor Mammedov; +Cc: qemu-devel, mst
Igor Mammedov <imammedo@redhat.com> writes:
> 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>
> ---
> 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();
What's wrong with a simple assert(num_heads <= vq->vring.num)?
> }
> /* On success, callers read a descriptor at vq->last_avail_idx.
> * Make sure descriptor read does not bypass avail index read. */
[...]
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [Qemu-devel] [PATCH] virtio: abort on fatal error instead of just exiting
2016-06-29 12:49 ` Markus Armbruster
@ 2016-06-29 16:36 ` Igor Mammedov
2016-06-30 5:12 ` Markus Armbruster
0 siblings, 1 reply; 8+ messages in thread
From: Igor Mammedov @ 2016-06-29 16:36 UTC (permalink / raw)
To: Markus Armbruster; +Cc: qemu-devel, mst
On Wed, 29 Jun 2016 14:49:59 +0200
Markus Armbruster <armbru@redhat.com> wrote:
> Igor Mammedov <imammedo@redhat.com> writes:
>
> > 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>
> > ---
> > 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();
>
> What's wrong with a simple assert(num_heads <= vq->vring.num)?
Nothing, it should work to as we don't use NDEBUG.
My intent was to make core dump at the point and no to remove
error message
(though message's mostly useless for me as virtio is unfamiliar to me
and I had to dig into core dump to analyze issue).
>
> > }
> > /* On success, callers read a descriptor at vq->last_avail_idx.
> > * Make sure descriptor read does not bypass avail index read.
> > */
> [...]
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [Qemu-devel] [PATCH] virtio: abort on fatal error instead of just exiting
2016-06-29 16:36 ` Igor Mammedov
@ 2016-06-30 5:12 ` Markus Armbruster
2016-06-30 6:55 ` Igor Mammedov
0 siblings, 1 reply; 8+ messages in thread
From: Markus Armbruster @ 2016-06-30 5:12 UTC (permalink / raw)
To: Igor Mammedov; +Cc: qemu-devel, mst
Igor Mammedov <imammedo@redhat.com> writes:
> On Wed, 29 Jun 2016 14:49:59 +0200
> Markus Armbruster <armbru@redhat.com> wrote:
>
>> Igor Mammedov <imammedo@redhat.com> writes:
>>
>> > 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>
>> > ---
>> > 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();
>>
>> What's wrong with a simple assert(num_heads <= vq->vring.num)?
> Nothing, it should work to as we don't use NDEBUG.
> My intent was to make core dump at the point and no to remove
> error message
> (though message's mostly useless for me as virtio is unfamiliar to me
> and I had to dig into core dump to analyze issue).
Understand.
The solution we really want is of course putting the device in an error
state, where it stays until reset.
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [Qemu-devel] [PATCH] virtio: abort on fatal error instead of just exiting
2016-06-30 5:12 ` Markus Armbruster
@ 2016-06-30 6:55 ` Igor Mammedov
0 siblings, 0 replies; 8+ messages in thread
From: Igor Mammedov @ 2016-06-30 6:55 UTC (permalink / raw)
To: Markus Armbruster; +Cc: qemu-devel, mst
On Thu, 30 Jun 2016 07:12:08 +0200
Markus Armbruster <armbru@redhat.com> wrote:
> Igor Mammedov <imammedo@redhat.com> writes:
>
> > On Wed, 29 Jun 2016 14:49:59 +0200
> > Markus Armbruster <armbru@redhat.com> wrote:
> >
> >> Igor Mammedov <imammedo@redhat.com> writes:
> >>
> >> > 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>
> >> > ---
> >> > 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();
> >>
> >> What's wrong with a simple assert(num_heads <= vq->vring.num)?
> > Nothing, it should work to as we don't use NDEBUG.
> > My intent was to make core dump at the point and no to remove
> > error message
> > (though message's mostly useless for me as virtio is unfamiliar to me
> > and I had to dig into core dump to analyze issue).
>
> Understand.
>
> The solution we really want is of course putting the device in an error
> state, where it stays until reset.
As Cornelia has mentioned earlier, there is/were a series on list for it,
aborting is just an interim step until those patches are ready.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH] virtio: abort on fatal error instead of just exiting
2016-06-28 8:24 [Qemu-devel] [PATCH] virtio: abort on fatal error instead of just exiting Igor Mammedov
` (2 preceding siblings ...)
2016-06-29 12:49 ` Markus Armbruster
@ 2016-07-27 18:13 ` Michael S. Tsirkin
3 siblings, 0 replies; 8+ messages in thread
From: Michael S. Tsirkin @ 2016-07-27 18:13 UTC (permalink / raw)
To: Igor Mammedov; +Cc: qemu-devel
On Tue, Jun 28, 2016 at 10:24:29AM +0200, Igor Mammedov 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>
I thought about this, and I'd rather we made the jump to
stopping the device straight away. Let's leave this alone
and focus on the real thing post 2.7.
E.g. creating coredumps from guests could cause some
disk to fill up. I'd rather not go there.
> ---
> 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);
>
> --
> 1.8.3.1
^ permalink raw reply [flat|nested] 8+ messages in thread