| Message ID | 1460689374-9690-3-git-send-email-saxenap.ltc@gmail.com |
|---|---|
| State | New |
| Headers | show |
On Fri, Apr 15, 2016 at 08:32:54AM +0530, Prerna Saxena wrote: > diff --git a/block/block-backend.c b/block/block-backend.c > index d74f670..0aa8692 100644 > --- a/block/block-backend.c > +++ b/block/block-backend.c > @@ -407,6 +407,7 @@ BlockBackend *blk_by_legacy_dinfo(DriveInfo *dinfo) > return blk; > } > } > + error_report("Drive Info not found, Aborting."); error_report() documentation says: * Format arguments like sprintf(). The resulting message should be a * single phrase, with no newline or trailing punctuation. > abort(); > } > > @@ -463,6 +464,8 @@ int blk_attach_dev(BlockBackend *blk, void *dev) > void blk_attach_dev_nofail(BlockBackend *blk, void *dev) > { > if (blk_attach_dev(blk, dev) < 0) { > + error_report("Attaching device model to block %s failed. Aborting", Please use '%s' instead of just %s for consistency with other error messages. > + blk->name); > abort(); > } > } > @@ -1143,6 +1146,7 @@ BlockErrorAction blk_get_error_action(BlockBackend *blk, bool is_read, > case BLOCKDEV_ON_ERROR_IGNORE: > return BLOCK_ERROR_ACTION_IGNORE; > default: > + error_report("Unrecognized Block Error Action %d. Aborting.",on_err); s/,/, / Please use scripts/checkpatch.pl to check for coding style violations. > diff --git a/exec.c b/exec.c > index c4f9036..1b1d713 100644 > --- a/exec.c > +++ b/exec.c > @@ -2218,6 +2218,7 @@ static MemTxResult subpage_read(void *opaque, hwaddr addr, uint64_t *data, > *data = ldq_p(buf); > return MEMTX_OK; > default: > + printf("Unsupported read size %d, Aborting", len); Why use printf() instead of error_report() in this file?
On 04/14/2016 09:02 PM, Prerna Saxena wrote: > Qemu code has abort() calls in various places which raises a SIGABRT; > This patch adds error messages before (most)calls to abort(), so that > it is easier to determine why QEMU died. The subject line says you are adding messages before debug(), but the rest of the patch is adding message before abort(). You'll need to fix that. Also, subject lines usually don't end in '.' > +++ b/block.c > @@ -3725,6 +3725,7 @@ void bdrv_remove_aio_context_notifier(BlockDriverState *bs, > } > } > > + error_report("Matching context notifier not found for removal. Aborting"); > abort(); The "Aborting" part of the message is redundant; it's pretty obvious that qemu aborted. I also wonder if you should be using g_assert_not_reached() instead of abort() in some (all?) of the places touched in this patch - at which point you don't have to worry about inventing a message that will never be printed. The reason I suggest it is that g_assert_not_reached() is self-documenting, and prints a nicer message than abort() if it does accidentally get reached.
Hi Eric, Thank you for the review. On Wed, Apr 27, 2016 at 9:30 PM, Eric Blake <eblake@redhat.com> wrote: > On 04/14/2016 09:02 PM, Prerna Saxena wrote: > > Qemu code has abort() calls in various places which raises a SIGABRT; > > This patch adds error messages before (most)calls to abort(), so that > > it is easier to determine why QEMU died. > > The subject line says you are adding messages before debug(), but the > rest of the patch is adding message before abort(). You'll need to fix > that. Also, subject lines usually don't end in '.' > Sorry, the subject line shouldve said : "Add error messages before abort()". Will resend. > > > +++ b/block.c > > @@ -3725,6 +3725,7 @@ void > bdrv_remove_aio_context_notifier(BlockDriverState *bs, > > } > > } > > > > + error_report("Matching context notifier not found for removal. > Aborting"); > > abort(); > > The "Aborting" part of the message is redundant; it's pretty obvious > that qemu aborted. > > Agree. > I also wonder if you should be using g_assert_not_reached() instead of > abort() in some (all?) of the places touched in this patch - at which > point you don't have to worry about inventing a message that will never > be printed. The reason I suggest it is that g_assert_not_reached() is > self-documenting, and prints a nicer message than abort() if it does > accidentally get reached. > > Ah, thanks for this suggestion. I will look up g_assert_not_reached() to see how I can use it (at all places, possibly). Regards, Prerna > -- > Eric Blake eblake redhat com +1-919-301-3266 > Libvirt virtualization library http://libvirt.org > >
Eric Blake <eblake@redhat.com> writes: > On 04/14/2016 09:02 PM, Prerna Saxena wrote: >> Qemu code has abort() calls in various places which raises a SIGABRT; >> This patch adds error messages before (most)calls to abort(), so that >> it is easier to determine why QEMU died. > > The subject line says you are adding messages before debug(), but the > rest of the patch is adding message before abort(). You'll need to fix > that. Also, subject lines usually don't end in '.' > >> +++ b/block.c >> @@ -3725,6 +3725,7 @@ void bdrv_remove_aio_context_notifier(BlockDriverState *bs, >> } >> } >> >> + error_report("Matching context notifier not found for removal. Aborting"); >> abort(); > > The "Aborting" part of the message is redundant; it's pretty obvious > that qemu aborted. > > I also wonder if you should be using g_assert_not_reached() instead of > abort() in some (all?) of the places touched in this patch - at which > point you don't have to worry about inventing a message that will never > be printed. The reason I suggest it is that g_assert_not_reached() is > self-documenting, and prints a nicer message than abort() if it does > accidentally get reached. Printing elaborate messages before killing the program on programming errors seems pointless. These errors should never happen. If they do, something's seriously wrong with the program, and you need professional help to debug it. Pretty much the only generally useful clue the dying program can provide for that is where the error is. abort() provides that clue (and more) if you let it dump core. assert() additionally provides that clue independently of the core. If you find users need more than that, chances are this error shouldn't abort() in the first place.
diff --git a/block.c b/block.c index d4939b4..160f277 100644 --- a/block.c +++ b/block.c @@ -3725,6 +3725,7 @@ void bdrv_remove_aio_context_notifier(BlockDriverState *bs, } } + error_report("Matching context notifier not found for removal. Aborting"); abort(); } diff --git a/block/block-backend.c b/block/block-backend.c index d74f670..0aa8692 100644 --- a/block/block-backend.c +++ b/block/block-backend.c @@ -407,6 +407,7 @@ BlockBackend *blk_by_legacy_dinfo(DriveInfo *dinfo) return blk; } } + error_report("Drive Info not found, Aborting."); abort(); } @@ -463,6 +464,8 @@ int blk_attach_dev(BlockBackend *blk, void *dev) void blk_attach_dev_nofail(BlockBackend *blk, void *dev) { if (blk_attach_dev(blk, dev) < 0) { + error_report("Attaching device model to block %s failed. Aborting", + blk->name); abort(); } } @@ -1143,6 +1146,7 @@ BlockErrorAction blk_get_error_action(BlockBackend *blk, bool is_read, case BLOCKDEV_ON_ERROR_IGNORE: return BLOCK_ERROR_ACTION_IGNORE; default: + error_report("Unrecognized Block Error Action %d. Aborting.",on_err); abort(); } } diff --git a/block/curl.c b/block/curl.c index 5a8f8b6..fe2225a 100644 --- a/block/curl.c +++ b/block/curl.c @@ -382,6 +382,7 @@ static void curl_multi_timeout_do(void *arg) curl_multi_check_completion(s); #else + error_report("Curl timer expired, Aborting."); abort(); #endif } diff --git a/block/io.c b/block/io.c index a7dbf85..6f45959 100644 --- a/block/io.c +++ b/block/io.c @@ -2045,6 +2045,7 @@ void bdrv_aio_cancel(BlockAIOCB *acb) } else if (acb->bs) { aio_poll(bdrv_get_aio_context(acb->bs), true); } else { + error_report("Aio context not found. Aborting."); abort(); } } diff --git a/block/linux-aio.c b/block/linux-aio.c index 805757e..38d7812 100644 --- a/block/linux-aio.c +++ b/block/linux-aio.c @@ -206,6 +206,7 @@ static void ioq_submit(struct qemu_laio_state *s) break; } if (ret < 0) { + error_report("Error %d submitting io. Aborting.", ret); abort(); } diff --git a/block/mirror.c b/block/mirror.c index c2cfc1a..600e3c2 100644 --- a/block/mirror.c +++ b/block/mirror.c @@ -389,6 +389,8 @@ static uint64_t coroutine_fn mirror_iteration(MirrorBlockJob *s) mirror_do_zero_or_discard(s, sector_num, io_sectors, true); break; default: + error_report("Unrecognized mirror option %d. Aborting.", + mirror_method); abort(); } assert(io_sectors); diff --git a/block/qcow2-cache.c b/block/qcow2-cache.c index 0fe8eda..80766a2 100644 --- a/block/qcow2-cache.c +++ b/block/qcow2-cache.c @@ -334,6 +334,7 @@ static int qcow2_cache_do_get(BlockDriverState *bs, Qcow2Cache *c, if (min_lru_index == -1) { /* This can't happen in current synchronous code, but leave the check * here as a reminder for whoever starts using AIO with the cache */ + error_report("Invalid Index %d, Aborting", min_lru_index); abort(); } diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c index 31ecc10..1914d97 100644 --- a/block/qcow2-cluster.c +++ b/block/qcow2-cluster.c @@ -583,6 +583,7 @@ int qcow2_get_cluster_offset(BlockDriverState *bs, uint64_t offset, } break; default: + error_report("Invalid cluster type %d. Aborting.", ret); abort(); } @@ -868,6 +869,7 @@ static int count_cow_clusters(BDRVQcow2State *s, int nb_clusters, case QCOW2_CLUSTER_ZERO: break; default: + error_report("Invalid cluster type %d, Aborting.", cluster_type); abort(); } } @@ -1494,6 +1496,7 @@ static int discard_single_l2(BlockDriverState *bs, uint64_t offset, break; default: + error_report("Invalid cluster type %d. Aborting.", ret); abort(); } diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c index ca6094f..6ff8395 100644 --- a/block/qcow2-refcount.c +++ b/block/qcow2-refcount.c @@ -28,6 +28,7 @@ #include "block/block_int.h" #include "block/qcow2.h" #include "qemu/range.h" +#include "qemu/error-report.h" static int64_t alloc_clusters_noref(BlockDriverState *bs, uint64_t size); static int QEMU_WARN_UNUSED_RESULT update_refcount(BlockDriverState *bs, @@ -1033,6 +1034,8 @@ void qcow2_free_any_clusters(BlockDriverState *bs, uint64_t l2_entry, case QCOW2_CLUSTER_UNALLOCATED: break; default: + error_report("Invalid cluster type %d, Aborting", + qcow2_get_cluster_type(l2_entry)); abort(); } } @@ -1170,6 +1173,8 @@ int qcow2_update_snapshot_refcount(BlockDriverState *bs, break; default: + error_report("Invalid cluster type %d, Aborting", + qcow2_get_cluster_type(offset)); abort(); } @@ -1470,6 +1475,8 @@ static int check_refcounts_l2(BlockDriverState *bs, BdrvCheckResult *res, break; default: + error_report("Invalid cluster type %d, Aborting", + qcow2_get_cluster_type(l2_entry)); abort(); } } diff --git a/block/qcow2.c b/block/qcow2.c index 470734b..7550fa9 100644 --- a/block/qcow2.c +++ b/block/qcow2.c @@ -3102,6 +3102,8 @@ static int qcow2_amend_options(BlockDriverState *bs, QemuOpts *opts, } else { /* if this point is reached, this probably means a new option was * added without having it covered here */ + error_report("Invalid qcow2 amend option %s. Aborting.", + desc->name); abort(); } diff --git a/blockdev.c b/blockdev.c index f1f520a..4bc001c 100644 --- a/blockdev.c +++ b/blockdev.c @@ -2556,6 +2556,8 @@ void qmp_blockdev_change_medium(const char *device, const char *filename, break; default: + error_report("Unrecognized change mode %d for block device, Aborting", + read_only); abort(); } @@ -3587,6 +3589,7 @@ void qmp_drive_mirror(const char *device, const char *target, NULL, size, flags, &local_err, false); break; default: + error_report("Unrecognized image mode %d, Aborting.", mode); abort(); } } diff --git a/crypto/aes.c b/crypto/aes.c index 3456eac..3228c5e 100644 --- a/crypto/aes.c +++ b/crypto/aes.c @@ -1162,6 +1162,7 @@ int AES_set_encrypt_key(const unsigned char *userKey, const int bits, rk += 8; } } + error_report("Error setting AES encryption key, Aborting."); abort(); } diff --git a/exec.c b/exec.c index c4f9036..1b1d713 100644 --- a/exec.c +++ b/exec.c @@ -2218,6 +2218,7 @@ static MemTxResult subpage_read(void *opaque, hwaddr addr, uint64_t *data, *data = ldq_p(buf); return MEMTX_OK; default: + printf("Unsupported read size %d, Aborting", len); abort(); } } @@ -2247,6 +2248,7 @@ static MemTxResult subpage_write(void *opaque, hwaddr addr, stq_p(buf, value); break; default: + printf("Unsupported write size %d, Aborting", len); abort(); } return address_space_write(subpage->as, addr + subpage->base, @@ -2620,6 +2622,7 @@ static MemTxResult address_space_write_continue(AddressSpace *as, hwaddr addr, attrs); break; default: + printf("Unsupported memory access size %d, Aborting.", l); abort(); } } else { @@ -2712,6 +2715,7 @@ MemTxResult address_space_read_continue(AddressSpace *as, hwaddr addr, stb_p(buf, val); break; default: + printf("Unsupported memory access size %d, Aborting.", l); abort(); } } else { diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c index c3ce54a..ef224e2 100644 --- a/hw/scsi/scsi-disk.c +++ b/hw/scsi/scsi-disk.c @@ -1845,6 +1845,7 @@ static void scsi_disk_emulate_write_data(SCSIRequest *req) break; default: + printf("Unhandled SCSI command %d, Aborting",req->cmd.buf[0]); abort(); } } @@ -2187,6 +2188,7 @@ static int32_t scsi_disk_dma_command(SCSIRequest *req, uint8_t *buf) r->sector_count = len * (s->qdev.blocksize / 512); break; default: + printf("Unhandled SCSI command %d, Aborting.",command); abort(); illegal_request: scsi_check_condition(r, SENSE_CODE(INVALID_FIELD)); diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c index f745c4a..5cd2efb 100644 --- a/hw/virtio/virtio.c +++ b/hw/virtio/virtio.c @@ -1147,8 +1147,10 @@ VirtQueue *virtio_add_queue(VirtIODevice *vdev, int queue_size, break; } - if (i == VIRTIO_QUEUE_MAX || queue_size > VIRTQUEUE_MAX_SIZE) + if (i == VIRTIO_QUEUE_MAX || queue_size > VIRTQUEUE_MAX_SIZE) { + error_report("Error adding virtqueue. Max limits reached. Aborting."); abort(); + } vdev->vq[i].vring.num = queue_size; vdev->vq[i].vring.num_default = queue_size; @@ -1162,6 +1164,7 @@ VirtQueue *virtio_add_queue(VirtIODevice *vdev, int queue_size, void virtio_del_queue(VirtIODevice *vdev, int n) { if (n < 0 || n >= VIRTIO_QUEUE_MAX) { + error_report("Error deleting virtqueue.Index out of range.Aborting"); abort(); } diff --git a/vl.c b/vl.c index 9df534f..3e6a686 100644 --- a/vl.c +++ b/vl.c @@ -2273,6 +2273,8 @@ char *qemu_find_file(int type, const char *name) subdir = "keymaps/"; break; default: + error_printf("Unrecognized type %d for file %s. Exiting.", + type, name); abort(); }
Qemu code has abort() calls in various places which raises a SIGABRT; This patch adds error messages before (most)calls to abort(), so that it is easier to determine why QEMU died. Signed-off-by: Prerna Saxena <saxenap.ltc@gmail.com> --- block.c | 1 + block/block-backend.c | 4 ++++ block/curl.c | 1 + block/io.c | 1 + block/linux-aio.c | 1 + block/mirror.c | 2 ++ block/qcow2-cache.c | 1 + block/qcow2-cluster.c | 3 +++ block/qcow2-refcount.c | 7 +++++++ block/qcow2.c | 2 ++ blockdev.c | 3 +++ crypto/aes.c | 1 + exec.c | 4 ++++ hw/scsi/scsi-disk.c | 2 ++ hw/virtio/virtio.c | 5 ++++- vl.c | 2 ++ 16 files changed, 39 insertions(+), 1 deletion(-)