Re: [PATCH v3 0/2] hw/block/block.c: improve confusing error

2024-01-30 Thread Stefan Hajnoczi
On Tue, Jan 30, 2024 at 09:30:30AM +0200, Manos Pitsidianakis wrote:
> In cases where a device tries to read more bytes than the block device
> contains with the blk_check_size_and_read_all() function, the error is
> vague: "device requires X bytes, block backend provides Y bytes".
> 
> This patch changes the errors of this function to include the block
> backend name, the device id and device type name where appropriate.
> 
> Version 3:
> - Changed phrasing "%s device with id='%s'" to "%s device '%s'" since
>   second parameter might be either device id or device path.
> (thanks Stefan Hajnoczi )
> 
> Version 2:
> - Assert dev is not NULL on qdev_get_human_name
> (thanks Phil Mathieu-Daudé )
> 
> Manos Pitsidianakis (2):
>   hw/core/qdev.c: add qdev_get_human_name()
>   hw/block/block.c: improve confusing blk_check_size_and_read_all()
> error
> 
>  include/hw/block/block.h |  4 ++--
>  include/hw/qdev-core.h   | 14 ++
>  hw/block/block.c | 25 +++--
>  hw/block/m25p80.c|  3 ++-
>  hw/block/pflash_cfi01.c  |  4 ++--
>  hw/block/pflash_cfi02.c  |  2 +-
>  hw/core/qdev.c   |  8 
>  7 files changed, 44 insertions(+), 16 deletions(-)
> 
> Range-diff against v2:
> 1:  5fb5879708 ! 1:  8b566bfced hw/core/qdev.c: add qdev_get_human_name()
> @@ Commit message
>  Add a simple method to return some kind of human readable identifier 
> for
>  use in error messages.
>  
> +Reviewed-by: Stefan Hajnoczi 
>  Signed-off-by: Manos Pitsidianakis 
>  
>   ## include/hw/qdev-core.h ##
> 2:  8e7eb17fbd ! 2:  7260eadff2 hw/block/block.c: improve confusing 
> blk_check_size_and_read_all() error
> @@ hw/block/block.c: static int blk_pread_nonzeroes(BlockBackend *blk, 
> hwaddr size,
>  -   "block backend provides %" PRIu64 " bytes",
>  -   size, blk_len);
>  +dev_id = qdev_get_human_name(dev);
> -+error_setg(errp, "%s device with id='%s' requires %" HWADDR_PRIu
> ++error_setg(errp, "%s device '%s' requires %" HWADDR_PRIu
>  +   " bytes, %s block backend provides %" PRIu64 " 
> bytes",
>  +   object_get_typename(OBJECT(dev)), dev_id, size,
>  +   blk_name(blk), blk_len);
> @@ hw/block/block.c: bool blk_check_size_and_read_all(BlockBackend *blk, 
> void *buf,
>  -error_setg_errno(errp, -ret, "can't read block backend");
>  +dev_id = qdev_get_human_name(dev);
>  +error_setg_errno(errp, -ret, "can't read %s block backend"
> -+ "for %s device with id='%s'",
> ++ " for %s device '%s'",
>  + blk_name(blk), 
> object_get_typename(OBJECT(dev)),
>  + dev_id);
>   return false;
> 
> base-commit: 11be70677c70fdccd452a3233653949b79e97908
> -- 
> γαῖα πυρί μιχθήτω
> 

Thanks, applied to my block tree:
https://gitlab.com/stefanha/qemu/commits/block

Stefan


signature.asc
Description: PGP signature


[PATCH v3 0/2] hw/block/block.c: improve confusing error

2024-01-29 Thread Manos Pitsidianakis
In cases where a device tries to read more bytes than the block device
contains with the blk_check_size_and_read_all() function, the error is
vague: "device requires X bytes, block backend provides Y bytes".

This patch changes the errors of this function to include the block
backend name, the device id and device type name where appropriate.

Version 3:
- Changed phrasing "%s device with id='%s'" to "%s device '%s'" since
  second parameter might be either device id or device path.
(thanks Stefan Hajnoczi )

Version 2:
- Assert dev is not NULL on qdev_get_human_name
(thanks Phil Mathieu-Daudé )

Manos Pitsidianakis (2):
  hw/core/qdev.c: add qdev_get_human_name()
  hw/block/block.c: improve confusing blk_check_size_and_read_all()
error

 include/hw/block/block.h |  4 ++--
 include/hw/qdev-core.h   | 14 ++
 hw/block/block.c | 25 +++--
 hw/block/m25p80.c|  3 ++-
 hw/block/pflash_cfi01.c  |  4 ++--
 hw/block/pflash_cfi02.c  |  2 +-
 hw/core/qdev.c   |  8 
 7 files changed, 44 insertions(+), 16 deletions(-)

Range-diff against v2:
1:  5fb5879708 ! 1:  8b566bfced hw/core/qdev.c: add qdev_get_human_name()
@@ Commit message
 Add a simple method to return some kind of human readable identifier 
for
 use in error messages.
 
+Reviewed-by: Stefan Hajnoczi 
 Signed-off-by: Manos Pitsidianakis 
 
  ## include/hw/qdev-core.h ##
2:  8e7eb17fbd ! 2:  7260eadff2 hw/block/block.c: improve confusing 
blk_check_size_and_read_all() error
@@ hw/block/block.c: static int blk_pread_nonzeroes(BlockBackend *blk, 
hwaddr size,
 -   "block backend provides %" PRIu64 " bytes",
 -   size, blk_len);
 +dev_id = qdev_get_human_name(dev);
-+error_setg(errp, "%s device with id='%s' requires %" HWADDR_PRIu
++error_setg(errp, "%s device '%s' requires %" HWADDR_PRIu
 +   " bytes, %s block backend provides %" PRIu64 " bytes",
 +   object_get_typename(OBJECT(dev)), dev_id, size,
 +   blk_name(blk), blk_len);
@@ hw/block/block.c: bool blk_check_size_and_read_all(BlockBackend *blk, 
void *buf,
 -error_setg_errno(errp, -ret, "can't read block backend");
 +dev_id = qdev_get_human_name(dev);
 +error_setg_errno(errp, -ret, "can't read %s block backend"
-+ "for %s device with id='%s'",
++ " for %s device '%s'",
 + blk_name(blk), object_get_typename(OBJECT(dev)),
 + dev_id);
  return false;

base-commit: 11be70677c70fdccd452a3233653949b79e97908
-- 
γαῖα πυρί μιχθήτω