[Qemu-devel] [PATCH 0/2] Cleanup and instrumenting qemu exits due to abort().

[Qemu-devel] [PATCH 0/2] Cleanup and instrumenting qemu exits due to abort().

[Prerna Saxena]

Today, some calls to abort() do not have a preceding error string that
might hint to the end user why QEMU died. Debugging such scenarios is
painful today.

This patchset attempts to clean up some dead code in vvfat.c;
it also aims to improve qemu error-reporting by placing error messages
that precede calls to abort().

Prerna Saxena (2):
  Block: Cleanup vvfat.c to remove dead code.
  Debug : Add error messages before a call to debug().

 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 ++
 block/vvfat.c          | 17 +++--------------
 blockdev.c             |  3 +++
 crypto/aes.c           |  1 +
 exec.c                 |  4 ++++
 hw/scsi/scsi-disk.c    |  2 ++
 hw/virtio/virtio.c     |  5 ++++-
 vl.c                   |  2 ++
 17 files changed, 42 insertions(+), 15 deletions(-)

-- 
1.8.1.2

[Qemu-devel] [PATCH 1/2] Block: Cleanup vvfat.c to remove dead code.

[Prerna Saxena]

Commit 43dc2a64 replaced assert() with abort(), but didnt remove statements
that followed these calls. So current code still has return values set after
a call to abort(). Such statements will never execute and need to be cleaned
up.

Signed-off-by: Prerna Saxena <saxenap.ltc@nutanix.com>
---
 block/vvfat.c | 17 +++--------------
 1 file changed, 3 insertions(+), 14 deletions(-)

diff --git a/block/vvfat.c b/block/vvfat.c
index 6b85314..ffe739b 100644
--- a/block/vvfat.c
+++ b/block/vvfat.c
@@ -1747,8 +1747,7 @@ static uint32_t get_cluster_count_for_direntry(BDRVVVFATState* s,
 	    schedule_new_file(s, g_strdup(path), cluster_num);
 	else {
             abort();
-	    return 0;
-	}
+	    }
     }
 
     while(1) {
@@ -1768,7 +1767,6 @@ static uint32_t get_cluster_count_for_direntry(BDRVVVFATState* s,
 			    * (cluster_num - mapping->begin)) {
 			/* offset of this cluster in file chain has changed */
                         abort();
-			copy_it = 1;
 		    } else if (offset == 0) {
 			const char* basename = get_basename(mapping->path);
 
@@ -1780,7 +1778,6 @@ static uint32_t get_cluster_count_for_direntry(BDRVVVFATState* s,
 		    if (mapping->first_mapping_index != first_mapping_index
 			    && mapping->info.file.offset > 0) {
                         abort();
-			copy_it = 1;
 		    }
 
 		    /* need to write out? */
@@ -1946,8 +1943,6 @@ DLOG(fprintf(stderr, "check direntry %d:\n", i); print_direntry(direntries + i))
 		}
 	    } else
                 abort(); /* cluster_count = 0; */
-
-	    ret += cluster_count;
 	}
 
 	cluster_num = modified_fat_get(s, cluster_num);
@@ -2578,10 +2573,6 @@ static int handle_commits(BDRVVVFATState* s)
     for (i = 0; !fail && i < s->commits.next; i++) {
 	commit_t* commit = array_get(&(s->commits), i);
 	switch(commit->action) {
-	case ACTION_RENAME: case ACTION_MKDIR:
-            abort();
-	    fail = -2;
-	    break;
 	case ACTION_WRITEOUT: {
 #ifndef NDEBUG
             /* these variables are only used by assert() below */
@@ -2639,6 +2630,8 @@ static int handle_commits(BDRVVVFATState* s)
 
 	    break;
 	}
+    case ACTION_RENAME:
+    case ACTION_MKDIR:
 	default:
             abort();
 	}
@@ -2729,7 +2722,6 @@ static int do_commit(BDRVVVFATState* s)
     if (ret) {
 	fprintf(stderr, "Error handling renames (%d)\n", ret);
         abort();
-	return ret;
     }
 
     /* copy FAT (with bdrv_read) */
@@ -2740,21 +2732,18 @@ static int do_commit(BDRVVVFATState* s)
     if (ret) {
 	fprintf(stderr, "Fatal: error while committing (%d)\n", ret);
         abort();
-	return ret;
     }
 
     ret = handle_commits(s);
     if (ret) {
 	fprintf(stderr, "Error handling commits (%d)\n", ret);
         abort();
-	return ret;
     }
 
     ret = handle_deletes(s);
     if (ret) {
 	fprintf(stderr, "Error deleting\n");
         abort();
-	return ret;
     }
 
     if (s->qcow->drv->bdrv_make_empty) {
-- 
1.8.1.2

Re: [Qemu-devel] [PATCH 1/2] Block: Cleanup vvfat.c to remove dead code.

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1143 bytes --]

On Fri, Apr 15, 2016 at 08:32:53AM +0530, Prerna Saxena wrote:
> Commit 43dc2a64 replaced assert() with abort(), but didnt remove statements
> that followed these calls. So current code still has return values set after
> a call to abort(). Such statements will never execute and need to be cleaned
> up.
> 
> Signed-off-by: Prerna Saxena <saxenap.ltc@nutanix.com>
> ---
>  block/vvfat.c | 17 +++--------------
>  1 file changed, 3 insertions(+), 14 deletions(-)
> 
> diff --git a/block/vvfat.c b/block/vvfat.c
> index 6b85314..ffe739b 100644
> --- a/block/vvfat.c
> +++ b/block/vvfat.c
> @@ -1747,8 +1747,7 @@ static uint32_t get_cluster_count_for_direntry(BDRVVVFATState* s,
>  	    schedule_new_file(s, g_strdup(path), cluster_num);
>  	else {
>              abort();
> -	    return 0;
> -	}
> +	    }

The following change breaks indentation:

  ^Ielse {
  ...
  -^I}
  +^I    }

Since the else statement only has a single tab character it's wrong to
have mixed tabs and spaces for the closing curly.

Please resend this patch with tab-only indentation.  The diff hunks
below sometimes introduce spaces.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [PATCH 1/2] Block: Cleanup vvfat.c to remove dead code.

[Alex Bennée]

Prerna Saxena <saxenap.ltc@gmail.com> writes:

> Commit 43dc2a64 replaced assert() with abort(), but didnt remove statements
> that followed these calls. So current code still has return values set after
> a call to abort(). Such statements will never execute and need to be cleaned
> up.
>
> Signed-off-by: Prerna Saxena <saxenap.ltc@nutanix.com>
> ---
>  block/vvfat.c | 17 +++--------------
>  1 file changed, 3 insertions(+), 14 deletions(-)
>
> diff --git a/block/vvfat.c b/block/vvfat.c
> index 6b85314..ffe739b 100644
> --- a/block/vvfat.c
> +++ b/block/vvfat.c
> @@ -1747,8 +1747,7 @@ static uint32_t get_cluster_count_for_direntry(BDRVVVFATState* s,
>  	    schedule_new_file(s, g_strdup(path), cluster_num);
>  	else {
>              abort();
> -	    return 0;
> -	}
> +	    }
>      }
>
>      while(1) {
> @@ -1768,7 +1767,6 @@ static uint32_t get_cluster_count_for_direntry(BDRVVVFATState* s,
>  			    * (cluster_num - mapping->begin)) {
>  			/* offset of this cluster in file chain has changed */
>                          abort();
> -			copy_it = 1;
>  		    } else if (offset == 0) {
>  			const char* basename = get_basename(mapping->path);
>
> @@ -1780,7 +1778,6 @@ static uint32_t get_cluster_count_for_direntry(BDRVVVFATState* s,
>  		    if (mapping->first_mapping_index != first_mapping_index
>  			    && mapping->info.file.offset > 0) {
>                          abort();
> -			copy_it = 1;
>  		    }
>
>  		    /* need to write out? */
> @@ -1946,8 +1943,6 @@ DLOG(fprintf(stderr, "check direntry %d:\n", i); print_direntry(direntries + i))
>  		}
>  	    } else
>                  abort(); /* cluster_count = 0; */
> -
> -	    ret += cluster_count;

I don't think you meant to do this. The ret += is not part of the else leg.

>  	}
>
>  	cluster_num = modified_fat_get(s, cluster_num);
> @@ -2578,10 +2573,6 @@ static int handle_commits(BDRVVVFATState* s)
>      for (i = 0; !fail && i < s->commits.next; i++) {
>  	commit_t* commit = array_get(&(s->commits), i);
>  	switch(commit->action) {
> -	case ACTION_RENAME: case ACTION_MKDIR:
> -            abort();
> -	    fail = -2;
> -	    break;
>  	case ACTION_WRITEOUT: {
>  #ifndef NDEBUG
>              /* these variables are only used by assert() below */
> @@ -2639,6 +2630,8 @@ static int handle_commits(BDRVVVFATState* s)
>
>  	    break;
>  	}
> +    case ACTION_RENAME:
> +    case ACTION_MKDIR:
>  	default:
>              abort();
>  	}
> @@ -2729,7 +2722,6 @@ static int do_commit(BDRVVVFATState* s)
>      if (ret) {
>  	fprintf(stderr, "Error handling renames (%d)\n", ret);
>          abort();
> -	return ret;
>      }
>
>      /* copy FAT (with bdrv_read) */
> @@ -2740,21 +2732,18 @@ static int do_commit(BDRVVVFATState* s)
>      if (ret) {
>  	fprintf(stderr, "Fatal: error while committing (%d)\n", ret);
>          abort();
> -	return ret;
>      }
>
>      ret = handle_commits(s);
>      if (ret) {
>  	fprintf(stderr, "Error handling commits (%d)\n", ret);
>          abort();
> -	return ret;
>      }
>
>      ret = handle_deletes(s);
>      if (ret) {
>  	fprintf(stderr, "Error deleting\n");
>          abort();
> -	return ret;
>      }
>
>      if (s->qcow->drv->bdrv_make_empty) {


--
Alex Bennée

[Qemu-devel] [PATCH 2/2] Debug : Add error messages before a call to debug().

[Prerna Saxena]

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(-)

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();
     }
 
-- 
1.8.1.2

Re: [Qemu-devel] [PATCH 2/2] Debug : Add error messages before a call to debug().

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1784 bytes --]

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?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [PATCH 2/2] Debug : Add error messages before a call to debug().

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1257 bytes --]

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.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH 2/2] Debug : Add error messages before a call to debug().

[Prerna]

[-- Attachment #1: Type: text/plain, Size: 1641 bytes --]

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
>
>

[-- Attachment #2: Type: text/html, Size: 2855 bytes --]

Re: [Qemu-devel] [PATCH 2/2] Debug : Add error messages before a call to debug().

[Markus Armbruster]

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.