| Message ID | 1467102269-11112-1-git-send-email-imammedo@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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); >
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>
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. */ [...]
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. > > */ > [...]
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.
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.
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
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);
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(-)