diff mbox series

[RFC,1/5] hw/block: blk_check_size_and_read_all should report backend name

Message ID 20201116104216.439650-2-david.edmondson@oracle.com (mailing list archive)
State New, archived
Headers show
Series ARM: reduce the memory consumed when mapping UEFI flash images | expand

Commit Message

David Edmondson Nov. 16, 2020, 10:42 a.m. UTC
If there are problems examining or reading data from the block
backend, the error messages should include an appropriate identifier
to assist in diagnoses.

Signed-off-by: David Edmondson <david.edmondson@oracle.com>
---
 hw/block/block.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

Comments

Philippe Mathieu-Daudé Nov. 16, 2020, 11:23 a.m. UTC | #1
On 11/16/20 11:42 AM, David Edmondson wrote:
> If there are problems examining or reading data from the block
> backend, the error messages should include an appropriate identifier
> to assist in diagnoses.
> 
> Signed-off-by: David Edmondson <david.edmondson@oracle.com>
> ---
>  hw/block/block.c | 14 +++++++-------
>  1 file changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/hw/block/block.c b/hw/block/block.c
> index 1e34573da7..8b284e1f14 100644
> --- a/hw/block/block.c
> +++ b/hw/block/block.c
> @@ -20,9 +20,6 @@
>   * BDRV_REQUEST_MAX_BYTES.
>   * On success, return true.
>   * On failure, store an error through @errp and return false.
> - * Note that the error messages do not identify the block backend.
> - * TODO Since callers don't either, this can result in confusing
> - * errors.
>   * This function not intended for actual block devices, which read on
>   * demand.  It's for things like memory devices that (ab)use a block
>   * backend to provide persistence.
> @@ -32,17 +29,19 @@ bool blk_check_size_and_read_all(BlockBackend *blk, void *buf, hwaddr size,
>  {
>      int64_t blk_len;
>      int ret;
> +    const char *name = blk_name(blk);
>  
>      blk_len = blk_getlength(blk);
>      if (blk_len < 0) {
>          error_setg_errno(errp, -blk_len,
> -                         "can't get size of block backend");
> +                         "can't get size of block backend %s",

Maybe '%s' to notice empty name?

> +                         name);
>          return false;
>      }
>      if (blk_len != size) {
>          error_setg(errp, "device requires %" HWADDR_PRIu " bytes, "
> -                   "block backend provides %" PRIu64 " bytes",
> -                   size, blk_len);
> +                   "block backend %s provides %" PRIu64 " bytes",
> +                   size, name, blk_len);
>          return false;
>      }
>  
> @@ -55,7 +54,8 @@ bool blk_check_size_and_read_all(BlockBackend *blk, void *buf, hwaddr size,
>      assert(size <= BDRV_REQUEST_MAX_BYTES);
>      ret = blk_pread(blk, 0, buf, size);
>      if (ret < 0) {
> -        error_setg_errno(errp, -ret, "can't read block backend");
> +        error_setg_errno(errp, -ret, "can't read block backend %s",
> +                         name);
>          return false;
>      }
>      return true;
>
David Edmondson Nov. 16, 2020, 1:29 p.m. UTC | #2
On Monday, 2020-11-16 at 12:23:24 +01, Philippe Mathieu-Daudé wrote:

> On 11/16/20 11:42 AM, David Edmondson wrote:
>> If there are problems examining or reading data from the block
>> backend, the error messages should include an appropriate identifier
>> to assist in diagnoses.
>> 
>> Signed-off-by: David Edmondson <david.edmondson@oracle.com>
>> ---
>>  hw/block/block.c | 14 +++++++-------
>>  1 file changed, 7 insertions(+), 7 deletions(-)
>> 
>> diff --git a/hw/block/block.c b/hw/block/block.c
>> index 1e34573da7..8b284e1f14 100644
>> --- a/hw/block/block.c
>> +++ b/hw/block/block.c
>> @@ -20,9 +20,6 @@
>>   * BDRV_REQUEST_MAX_BYTES.
>>   * On success, return true.
>>   * On failure, store an error through @errp and return false.
>> - * Note that the error messages do not identify the block backend.
>> - * TODO Since callers don't either, this can result in confusing
>> - * errors.
>>   * This function not intended for actual block devices, which read on
>>   * demand.  It's for things like memory devices that (ab)use a block
>>   * backend to provide persistence.
>> @@ -32,17 +29,19 @@ bool blk_check_size_and_read_all(BlockBackend *blk, void *buf, hwaddr size,
>>  {
>>      int64_t blk_len;
>>      int ret;
>> +    const char *name = blk_name(blk);
>>  
>>      blk_len = blk_getlength(blk);
>>      if (blk_len < 0) {
>>          error_setg_errno(errp, -blk_len,
>> -                         "can't get size of block backend");
>> +                         "can't get size of block backend %s",
>
> Maybe '%s' to notice empty name?

Okay.

>> +                         name);
>>          return false;
>>      }
>>      if (blk_len != size) {
>>          error_setg(errp, "device requires %" HWADDR_PRIu " bytes, "
>> -                   "block backend provides %" PRIu64 " bytes",
>> -                   size, blk_len);
>> +                   "block backend %s provides %" PRIu64 " bytes",
>> +                   size, name, blk_len);
>>          return false;
>>      }
>>  
>> @@ -55,7 +54,8 @@ bool blk_check_size_and_read_all(BlockBackend *blk, void *buf, hwaddr size,
>>      assert(size <= BDRV_REQUEST_MAX_BYTES);
>>      ret = blk_pread(blk, 0, buf, size);
>>      if (ret < 0) {
>> -        error_setg_errno(errp, -ret, "can't read block backend");
>> +        error_setg_errno(errp, -ret, "can't read block backend %s",
>> +                         name);
>>          return false;
>>      }
>>      return true;
>> 

dme.
Alex Bennée Nov. 19, 2020, 11:56 a.m. UTC | #3
David Edmondson <david.edmondson@oracle.com> writes:

> If there are problems examining or reading data from the block
> backend, the error messages should include an appropriate identifier
> to assist in diagnoses.
>
> Signed-off-by: David Edmondson <david.edmondson@oracle.com>

With Phillipe's suggested ''s:

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
diff mbox series

Patch

diff --git a/hw/block/block.c b/hw/block/block.c
index 1e34573da7..8b284e1f14 100644
--- a/hw/block/block.c
+++ b/hw/block/block.c
@@ -20,9 +20,6 @@ 
  * BDRV_REQUEST_MAX_BYTES.
  * On success, return true.
  * On failure, store an error through @errp and return false.
- * Note that the error messages do not identify the block backend.
- * TODO Since callers don't either, this can result in confusing
- * errors.
  * This function not intended for actual block devices, which read on
  * demand.  It's for things like memory devices that (ab)use a block
  * backend to provide persistence.
@@ -32,17 +29,19 @@  bool blk_check_size_and_read_all(BlockBackend *blk, void *buf, hwaddr size,
 {
     int64_t blk_len;
     int ret;
+    const char *name = blk_name(blk);
 
     blk_len = blk_getlength(blk);
     if (blk_len < 0) {
         error_setg_errno(errp, -blk_len,
-                         "can't get size of block backend");
+                         "can't get size of block backend %s",
+                         name);
         return false;
     }
     if (blk_len != size) {
         error_setg(errp, "device requires %" HWADDR_PRIu " bytes, "
-                   "block backend provides %" PRIu64 " bytes",
-                   size, blk_len);
+                   "block backend %s provides %" PRIu64 " bytes",
+                   size, name, blk_len);
         return false;
     }
 
@@ -55,7 +54,8 @@  bool blk_check_size_and_read_all(BlockBackend *blk, void *buf, hwaddr size,
     assert(size <= BDRV_REQUEST_MAX_BYTES);
     ret = blk_pread(blk, 0, buf, size);
     if (ret < 0) {
-        error_setg_errno(errp, -ret, "can't read block backend");
+        error_setg_errno(errp, -ret, "can't read block backend %s",
+                         name);
         return false;
     }
     return true;