Re: [PATCH 08/10] qapi: Correct error message for 'vcpu_dirty_limit' parameter

[Zhao Liu]

On Tue, Mar 12, 2024 at 03:13:41PM +0100, Markus Armbruster wrote:
> Date: Tue, 12 Mar 2024 15:13:41 +0100
> From: Markus Armbruster 
> Subject: [PATCH 08/10] qapi: Correct error message for 'vcpu_dirty_limit'
>  parameter
> 
> From: Philippe Mathieu-Daudé 
> 
> QERR_INVALID_PARAMETER_VALUE is defined as:
> 
>   #define QERR_INVALID_PARAMETER_VALUE \
>   "Parameter '%s' expects %s"
> 
> The current error is formatted as:
> 
>   "Parameter 'vcpu_dirty_limit' expects is invalid, it must greater then 1 
> MB/s"
> 
> Replace by:
> 
>   "Parameter 'vcpu_dirty_limit' is invalid, it must greater than 1 MB/s"

Is there a grammar error here? Maybe

s/it must greater/it must be greater/

> Signed-off-by: Philippe Mathieu-Daudé 
> Reviewed-by: Juan Quintela 
> Signed-off-by: Markus Armbruster 
> ---
>  migration/options.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/migration/options.c b/migration/options.c
> index 40eb930940..c5115f1b5c 100644
> --- a/migration/options.c
> +++ b/migration/options.c
> @@ -1264,9 +1264,8 @@ bool migrate_params_check(MigrationParameters *params, 
> Error **errp)
>  
>  if (params->has_vcpu_dirty_limit &&
>  (params->vcpu_dirty_limit < 1)) {
> -error_setg(errp, QERR_INVALID_PARAMETER_VALUE,
> -   "vcpu_dirty_limit",
> -   "is invalid, it must greater then 1 MB/s");
> +error_setg(errp, "Parameter 'vcpu_dirty_limit' is invalid,"
> + " it must greater than 1 MB/s");
>  return false;
>  }
>

Otherwise,

Reviewed-by: Zhao Liu 

Re: [PATCH 10/10] qapi: Inline and remove QERR_PROPERTY_VALUE_BAD definition

[Zhao Liu]

On Tue, Mar 12, 2024 at 03:13:43PM +0100, Markus Armbruster wrote:
> Date: Tue, 12 Mar 2024 15:13:43 +0100
> From: Markus Armbruster 
> Subject: [PATCH 10/10] qapi: Inline and remove QERR_PROPERTY_VALUE_BAD
>  definition
> 
> From: Philippe Mathieu-Daudé 
> 
> Address the comment added in commit 4629ed1e98
> ("qerror: Finally unused, clean up"), from 2015:
> 
>   /*
>* These macros will go away, please don't use
>* in new code, and do not add new ones!
>*/
> 
> Manual change. Remove the definition in
> include/qapi/qmp/qerror.h.
> 
> Signed-off-by: Philippe Mathieu-Daudé 
> Signed-off-by: Markus Armbruster 
> ---
>  include/qapi/qmp/qerror.h | 3 ---
>  hw/core/qdev-properties.c | 3 +--
>  2 files changed, 1 insertion(+), 5 deletions(-)

Reviewed-by: Zhao Liu 

Re: [PATCH 09/10] qapi: Inline and remove QERR_MIGRATION_ACTIVE definition

[Zhao Liu]

On Tue, Mar 12, 2024 at 03:13:42PM +0100, Markus Armbruster wrote:
> Date: Tue, 12 Mar 2024 15:13:42 +0100
> From: Markus Armbruster 
> Subject: [PATCH 09/10] qapi: Inline and remove QERR_MIGRATION_ACTIVE
>  definition
> 
> From: Philippe Mathieu-Daudé 
> 
> Address the comment added in commit 4629ed1e98
> ("qerror: Finally unused, clean up"), from 2015:
> 
>   /*
>* These macros will go away, please don't use
>* in new code, and do not add new ones!
>*/
> 
> Mechanical transformation using sed, manually
> removing the definition in include/qapi/qmp/qerror.h.
> 
> Signed-off-by: Philippe Mathieu-Daudé 
> Reviewed-by: Juan Quintela 
> Signed-off-by: Markus Armbruster 
> ---
>  include/qapi/qmp/qerror.h | 3 ---
>  migration/migration.c | 2 +-
>  migration/options.c   | 4 ++--
>  migration/savevm.c    | 2 +-
>  4 files changed, 4 insertions(+), 7 deletions(-)

Reviewed-by: Zhao Liu 

Re: [PATCH 07/10] qapi: Inline and remove QERR_INVALID_PARAMETER_TYPE definition

[Zhao Liu]

On Tue, Mar 12, 2024 at 03:13:40PM +0100, Markus Armbruster wrote:
> Date: Tue, 12 Mar 2024 15:13:40 +0100
> From: Markus Armbruster 
> Subject: [PATCH 07/10] qapi: Inline and remove QERR_INVALID_PARAMETER_TYPE
>  definition
> 
> From: Philippe Mathieu-Daudé 
> 
> Address the comment added in commit 4629ed1e98
> ("qerror: Finally unused, clean up"), from 2015:
> 
>   /*
>* These macros will go away, please don't use
>* in new code, and do not add new ones!
>*/
> 
> Manual changes (escaping the format in qapi/visit.py).
> 
> Signed-off-by: Philippe Mathieu-Daudé 
> Signed-off-by: Markus Armbruster 
> ---
>  include/qapi/qmp/qerror.h | 3 ---
>  qom/object.c  | 4 ++--
>  scripts/qapi/visit.py | 5 ++---
>  3 files changed, 4 insertions(+), 8 deletions(-)
>

Reviewed-by: Zhao Liu 

Re: [PATCH 06/10] qapi: Inline QERR_INVALID_PARAMETER_TYPE definition (constant value)

[Zhao Liu]

On Tue, Mar 12, 2024 at 03:13:39PM +0100, Markus Armbruster wrote:
> Date: Tue, 12 Mar 2024 15:13:39 +0100
> From: Markus Armbruster 
> Subject: [PATCH 06/10] qapi: Inline QERR_INVALID_PARAMETER_TYPE definition
>  (constant value)
> 
> From: Philippe Mathieu-Daudé 
> 
> Address the comment added in commit 4629ed1e98
> ("qerror: Finally unused, clean up"), from 2015:
> 
>   /*
>* These macros will go away, please don't use
>* in new code, and do not add new ones!
>*/
> 
> Mechanical transformation using the following
> coccinelle semantic patch:
> 
> @match@
> expression errp;
> expression param;
> constant value;
> @@
>  error_setg(errp, QERR_INVALID_PARAMETER_TYPE, param, value);
> 
> @script:python strformat depends on match@
> value << match.value;
> fixedfmt; // new var
> @@
> fixedfmt = f'"Invalid parameter type for \'%s\', expected: {value[1:-1]}"'
> coccinelle.fixedfmt = cocci.make_ident(fixedfmt)
> 
> @replace@
> expression match.errp;
> expression match.param;
> constant match.value;
> identifier strformat.fixedfmt;
> @@
> -error_setg(errp, QERR_INVALID_PARAMETER_TYPE, param, value);
> +error_setg(errp, fixedfmt, param);
> 
> Signed-off-by: Philippe Mathieu-Daudé 
> Signed-off-by: Markus Armbruster 
> ---
>  qapi/qobject-input-visitor.c | 32 
>  qapi/string-input-visitor.c  |  8 
>  qom/object.c | 12 
>  3 files changed, 28 insertions(+), 24 deletions(-)

Reviewed-by: Zhao Liu 

Re: [PATCH 05/10] qapi: Inline and remove QERR_INVALID_PARAMETER definition

[Zhao Liu]

On Tue, Mar 12, 2024 at 03:13:38PM +0100, Markus Armbruster wrote:
> Date: Tue, 12 Mar 2024 15:13:38 +0100
> From: Markus Armbruster 
> Subject: [PATCH 05/10] qapi: Inline and remove QERR_INVALID_PARAMETER
>  definition
> 
> From: Philippe Mathieu-Daudé 
> 
> Address the comment added in commit 4629ed1e98
> ("qerror: Finally unused, clean up"), from 2015:
> 
>   /*
>* These macros will go away, please don't use
>* in new code, and do not add new ones!
>*/
> 
> Mechanical transformation using:
> 
>   $ sed -i -e "s/QERR_INVALID_PARAMETER,/\"Invalid parameter '%s'\",/" \
> $(git grep -lw QERR_INVALID_PARAMETER)
> 
> Manually simplify qemu_opts_create(), and remove the macro definition
> in include/qapi/qmp/qerror.h.
> 
> Signed-off-by: Philippe Mathieu-Daudé 
> Signed-off-by: Markus Armbruster 
> ---
>  include/qapi/qmp/qerror.h |  3 ---
>  qapi/opts-visitor.c   |  2 +-
>  util/qemu-option.c| 10 +-
>  3 files changed, 6 insertions(+), 9 deletions(-)

Reviewed-by: Zhao Liu 

Re: [PATCH 04/10] qapi: Inline and remove QERR_DEVICE_NO_HOTPLUG definition

[Zhao Liu]

On Tue, Mar 12, 2024 at 03:13:37PM +0100, Markus Armbruster wrote:
> Date: Tue, 12 Mar 2024 15:13:37 +0100
> From: Markus Armbruster 
> Subject: [PATCH 04/10] qapi: Inline and remove QERR_DEVICE_NO_HOTPLUG
>  definition
> 
> From: Philippe Mathieu-Daudé 
> 
> Address the comment added in commit 4629ed1e98
> ("qerror: Finally unused, clean up"), from 2015:
> 
>   /*
>* These macros will go away, please don't use
>* in new code, and do not add new ones!
>*/
> 
> Mechanical transformation using sed, and manual cleanup.
> 
> Signed-off-by: Philippe Mathieu-Daudé 
> Signed-off-by: Markus Armbruster 
> ---
>  include/qapi/qmp/qerror.h | 3 ---
>  hw/core/qdev.c| 4 ++--
>  system/qdev-monitor.c     | 2 +-
>  3 files changed, 3 insertions(+), 6 deletions(-)

Reviewed-by: Zhao Liu 

Re: [PATCH 03/10] qapi: Inline and remove QERR_DEVICE_HAS_NO_MEDIUM definition

[Zhao Liu]

On Tue, Mar 12, 2024 at 03:13:36PM +0100, Markus Armbruster wrote:
> Date: Tue, 12 Mar 2024 15:13:36 +0100
> From: Markus Armbruster 
> Subject: [PATCH 03/10] qapi: Inline and remove QERR_DEVICE_HAS_NO_MEDIUM
>  definition
> 
> From: Philippe Mathieu-Daudé 
> 
> Address the comment added in commit 4629ed1e98
> ("qerror: Finally unused, clean up"), from 2015:
> 
>   /*
>* These macros will go away, please don't use
>* in new code, and do not add new ones!
>*/
> 
> Mechanical transformation using sed, and manual cleanup.
> 
> Signed-off-by: Philippe Mathieu-Daudé 
> Signed-off-by: Markus Armbruster 
> ---
>  include/qapi/qmp/qerror.h | 3 ---
>  block/snapshot.c  | 7 ---
>  blockdev.c| 2 +-
>  3 files changed, 5 insertions(+), 7 deletions(-)
>

Reviewed-by: Zhao Liu 

Re: [PATCH 02/10] qapi: Inline and remove QERR_BUS_NO_HOTPLUG definition

[Zhao Liu]

On Tue, Mar 12, 2024 at 03:13:35PM +0100, Markus Armbruster wrote:
> Date: Tue, 12 Mar 2024 15:13:35 +0100
> From: Markus Armbruster 
> Subject: [PATCH 02/10] qapi: Inline and remove QERR_BUS_NO_HOTPLUG
>  definition
> 
> From: Philippe Mathieu-Daudé 
> 
> Address the comment added in commit 4629ed1e98
> ("qerror: Finally unused, clean up"), from 2015:
> 
>   /*
>* These macros will go away, please don't use
>* in new code, and do not add new ones!
>*/
> 
> Mechanical transformation using sed, and manual cleanup.
> 
> Signed-off-by: Philippe Mathieu-Daudé 
> Reviewed-by: Cédric Le Goater 
> Signed-off-by: Markus Armbruster 
> ---
>  include/qapi/qmp/qerror.h | 3 ---
>  hw/ppc/spapr_pci.c| 5 ++---
>  system/qdev-monitor.c | 8 +++++---
>  3 files changed, 7 insertions(+), 9 deletions(-)
>

Reviewed-by: Zhao Liu 

Re: [PATCH 01/10] error: Drop superfluous #include "qapi/qmp/qerror.h"

[Zhao Liu]

On Tue, Mar 12, 2024 at 03:13:34PM +0100, Markus Armbruster wrote:
> Date: Tue, 12 Mar 2024 15:13:34 +0100
> From: Markus Armbruster 
> Subject: [PATCH 01/10] error: Drop superfluous #include "qapi/qmp/qerror.h"
> 
> Signed-off-by: Markus Armbruster 
> ---
>  backends/iommufd.c | 1 -
>  chardev/char-fe.c  | 1 -
>  system/rtc.c   | 1 -
>  3 files changed, 3 deletions(-)
> 

Reviewed-by: Zhao Liu 
 

[PATCH v2 04/29] block/copy-before-write: Fix missing ERRP_GUARD() for error_prepend()

[Zhao Liu]

From: Zhao Liu 

As the comment in qapi/error, passing @errp to error_prepend() requires
ERRP_GUARD():

* = Why, when and how to use ERRP_GUARD() =
*
* Without ERRP_GUARD(), use of the @errp parameter is restricted:
...
* - It should not be passed to error_prepend(), error_vprepend() or
*   error_append_hint(), because that doesn't work with _fatal.
* ERRP_GUARD() lifts these restrictions.
*
* To use ERRP_GUARD(), add it right at the beginning of the function.
* @errp can then be used without worrying about the argument being
* NULL or _fatal.

ERRP_GUARD() could avoid the case when @errp is _fatal, the user
can't see this additional information, because exit() happens in
error_setg earlier than information is added [1].

The cbw_open() passes @errp to error_prepend() without ERRP_GUARD().

Though it is the BlockDriver.bdrv_open() method, and currently its
@errp parameter only points to callers' local_err, to follow the
requirement of @errp, add missing ERRP_GUARD() at the beginning of this
function.

[1]: Issue description in the commit message of commit ae7c80a7bd73
 ("error: New macro ERRP_GUARD()").

Cc: John Snow 
Cc: Vladimir Sementsov-Ogievskiy 
Cc: Kevin Wolf 
Cc: Hanna Reitz 
Cc: qemu-block@nongnu.org
Signed-off-by: Zhao Liu 
Reviewed-by: Vladimir Sementsov-Ogievskiy 
---
 block/copy-before-write.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/block/copy-before-write.c b/block/copy-before-write.c
index 0842a1a6dfbe..8aba27a71d6d 100644
--- a/block/copy-before-write.c
+++ b/block/copy-before-write.c
@@ -407,6 +407,7 @@ out:
 static int cbw_open(BlockDriverState *bs, QDict *options, int flags,
 Error **errp)
 {
+ERRP_GUARD();
 BDRVCopyBeforeWriteState *s = bs->opaque;
 BdrvDirtyBitmap *bitmap = NULL;
 int64_t cluster_size;
-- 
2.34.1

[PATCH v2 10/29] block/snapshot: Fix missing ERRP_GUARD() for error_prepend()

[Zhao Liu]

From: Zhao Liu 

As the comment in qapi/error, passing @errp to error_prepend() requires
ERRP_GUARD():

* = Why, when and how to use ERRP_GUARD() =
*
* Without ERRP_GUARD(), use of the @errp parameter is restricted:
...
* - It should not be passed to error_prepend(), error_vprepend() or
*   error_append_hint(), because that doesn't work with _fatal.
* ERRP_GUARD() lifts these restrictions.
*
* To use ERRP_GUARD(), add it right at the beginning of the function.
* @errp can then be used without worrying about the argument being
* NULL or _fatal.

ERRP_GUARD() could avoid the case when @errp is _fatal, the user
can't see this additional information, because exit() happens in
error_setg earlier than information is added [1].

In block/snapshot.c, there are 2 functions passing @errp to
error_prepend() without ERRP_GUARD():
 - bdrv_all_delete_snapshot()
 - bdrv_all_goto_snapshot()

As the APIs exposed in include/block/snapshot.h, they could be called
by other modules.

To avoid potential issues as [1] said, add missing ERRP_GUARD() at the
beginning of these 2 functions.

[1]: Issue description in the commit message of commit ae7c80a7bd73
 ("error: New macro ERRP_GUARD()").

Cc: Kevin Wolf 
Cc: Hanna Reitz 
Cc: qemu-block@nongnu.org
Signed-off-by: Zhao Liu 
---
 block/snapshot.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/block/snapshot.c b/block/snapshot.c
index 8694fc0a3eba..8242b4abac41 100644
--- a/block/snapshot.c
+++ b/block/snapshot.c
@@ -566,6 +566,7 @@ int bdrv_all_delete_snapshot(const char *name,
  bool has_devices, strList *devices,
  Error **errp)
 {
+ERRP_GUARD();
 g_autoptr(GList) bdrvs = NULL;
 GList *iterbdrvs;
 
@@ -605,6 +606,7 @@ int bdrv_all_goto_snapshot(const char *name,
bool has_devices, strList *devices,
Error **errp)
 {
+ERRP_GUARD();
 g_autoptr(GList) bdrvs = NULL;
 GList *iterbdrvs;
 int ret;
-- 
2.34.1

[PATCH v2 13/29] block/virtio-blk: Fix missing ERRP_GUARD() for error_prepend()

[Zhao Liu]

From: Zhao Liu 

As the comment in qapi/error, passing @errp to error_prepend() requires
ERRP_GUARD():

* = Why, when and how to use ERRP_GUARD() =
*
* Without ERRP_GUARD(), use of the @errp parameter is restricted:
...
* - It should not be passed to error_prepend(), error_vprepend() or
*   error_append_hint(), because that doesn't work with _fatal.
* ERRP_GUARD() lifts these restrictions.
*
* To use ERRP_GUARD(), add it right at the beginning of the function.
* @errp can then be used without worrying about the argument being
* NULL or _fatal.

ERRP_GUARD() could avoid the case when @errp is _fatal, the user
can't see this additional information, because exit() happens in
error_setg earlier than information is added [1].

The virtio_blk_vq_aio_context_init() passes @errp to error_prepend().

Though its @errp points its caller's local @err variable, to follow the
requirement of @errp, add missing ERRP_GUARD() at the beginning of
virtio_blk_vq_aio_context_init().

[1]: Issue description in the commit message of commit ae7c80a7bd73
 ("error: New macro ERRP_GUARD()").

Cc: Stefan Hajnoczi 
Cc: "Michael S. Tsirkin" 
Cc: Kevin Wolf 
Cc: Hanna Reitz 
Cc: qemu-block@nongnu.org
Signed-off-by: Zhao Liu 
Reviewed-by: Stefan Hajnoczi 
Acked-by: Michael S. Tsirkin 
---
 hw/block/virtio-blk.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
index 738cb2ac367d..92de315f17f7 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -1682,6 +1682,7 @@ static bool apply_iothread_vq_mapping(
 /* Context: BQL held */
 static bool virtio_blk_vq_aio_context_init(VirtIOBlock *s, Error **errp)
 {
+ERRP_GUARD();
 VirtIODevice *vdev = VIRTIO_DEVICE(s);
 VirtIOBlkConf *conf = >conf;
 BusState *qbus = BUS(qdev_get_parent_bus(DEVICE(vdev)));
-- 
2.34.1

[PATCH v2 09/29] block/qed: Fix missing ERRP_GUARD() for error_prepend()

[Zhao Liu]

From: Zhao Liu 

As the comment in qapi/error, passing @errp to error_prepend() requires
ERRP_GUARD():

* = Why, when and how to use ERRP_GUARD() =
*
* Without ERRP_GUARD(), use of the @errp parameter is restricted:
...
* - It should not be passed to error_prepend(), error_vprepend() or
*   error_append_hint(), because that doesn't work with _fatal.
* ERRP_GUARD() lifts these restrictions.
*
* To use ERRP_GUARD(), add it right at the beginning of the function.
* @errp can then be used without worrying about the argument being
* NULL or _fatal.

ERRP_GUARD() could avoid the case when @errp is _fatal, the user
can't see this additional information, because exit() happens in
error_setg earlier than information is added [1].

The bdrv_qed_co_invalidate_cache() passes @errp to error_prepend()
without ERRP_GUARD().

Though it is a BlockDriver.bdrv_co_invalidate_cache() method, and
currently its @errp parameter only points to callers' local_err, to
follow the requirement of @errp, add missing ERRP_GUARD() at the
beginning of this function.

[1]: Issue description in the commit message of commit ae7c80a7bd73
 ("error: New macro ERRP_GUARD()").

Cc: Stefan Hajnoczi 
Cc: Kevin Wolf 
Cc: Hanna Reitz 
Cc: qemu-block@nongnu.org
Signed-off-by: Zhao Liu 
Reviewed-by: Stefan Hajnoczi 
---
 block/qed.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/block/qed.c b/block/qed.c
index bc2f0a61c0a9..fa5bc1108552 100644
--- a/block/qed.c
+++ b/block/qed.c
@@ -1579,6 +1579,7 @@ bdrv_qed_co_change_backing_file(BlockDriverState *bs, 
const char *backing_file,
 static void coroutine_fn GRAPH_RDLOCK
 bdrv_qed_co_invalidate_cache(BlockDriverState *bs, Error **errp)
 {
+ERRP_GUARD();
 BDRVQEDState *s = bs->opaque;
 int ret;
 
-- 
2.34.1

[PATCH v2 08/29] block/qcow2: Fix missing ERRP_GUARD() for error_prepend()

[Zhao Liu]

From: Zhao Liu 

As the comment in qapi/error, passing @errp to error_prepend() requires
ERRP_GUARD():

* = Why, when and how to use ERRP_GUARD() =
*
* Without ERRP_GUARD(), use of the @errp parameter is restricted:
...
* - It should not be passed to error_prepend(), error_vprepend() or
*   error_append_hint(), because that doesn't work with _fatal.
* ERRP_GUARD() lifts these restrictions.
*
* To use ERRP_GUARD(), add it right at the beginning of the function.
* @errp can then be used without worrying about the argument being
* NULL or _fatal.

ERRP_GUARD() could avoid the case when @errp is _fatal, the user
can't see this additional information, because exit() happens in
error_setg earlier than information is added [1].

In block/qcow2.c, there are 2 functions passing @errp to error_prepend()
without ERRP_GUARD():
 - qcow2_co_create()
 - qcow2_co_truncate()

There are too many possible callers to check the impact of the defect;
it may or may not be harmless. Thus it is necessary to protect @errp with
ERRP_GUARD().

Therefore, to avoid the issue like [1] said, add missing ERRP_GUARD() at
their beginning.

[1]: Issue description in the commit message of commit ae7c80a7bd73
 ("error: New macro ERRP_GUARD()").

Cc: Kevin Wolf 
Cc: Hanna Reitz 
Cc: qemu-block@nongnu.org
Signed-off-by: Zhao Liu 
Reviewed-by: Eric Blake 
---
v2:
 * Use Markus' sentence to polish commit message. (Markus)
 * Fix typo. (Eric)
---
 block/qcow2.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/block/qcow2.c b/block/qcow2.c
index 204f5854cff2..956128b40948 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -3483,6 +3483,7 @@ static uint64_t qcow2_opt_get_refcount_bits_del(QemuOpts 
*opts, int version,
 static int coroutine_fn GRAPH_UNLOCKED
 qcow2_co_create(BlockdevCreateOptions *create_options, Error **errp)
 {
+ERRP_GUARD();
 BlockdevCreateOptionsQcow2 *qcow2_opts;
 QDict *options;
 
@@ -4283,6 +4284,7 @@ static int coroutine_fn GRAPH_RDLOCK
 qcow2_co_truncate(BlockDriverState *bs, int64_t offset, bool exact,
   PreallocMode prealloc, BdrvRequestFlags flags, Error **errp)
 {
+ERRP_GUARD();
 BDRVQcow2State *s = bs->opaque;
 uint64_t old_length;
 int64_t new_l1_size;
-- 
2.34.1

[PATCH v2 05/29] block/nbd: Fix missing ERRP_GUARD() for error_prepend()

[Zhao Liu]

From: Zhao Liu 

As the comment in qapi/error, passing @errp to error_prepend() requires
ERRP_GUARD():

* = Why, when and how to use ERRP_GUARD() =
*
* Without ERRP_GUARD(), use of the @errp parameter is restricted:
...
* - It should not be passed to error_prepend(), error_vprepend() or
*   error_append_hint(), because that doesn't work with _fatal.
* ERRP_GUARD() lifts these restrictions.
*
* To use ERRP_GUARD(), add it right at the beginning of the function.
* @errp can then be used without worrying about the argument being
* NULL or _fatal.

ERRP_GUARD() could avoid the case when @errp is _fatal, the user
can't see this additional information, because exit() happens in
error_setg earlier than information is added [1].

The nbd_co_do_receive_one_chunk() passes @errp to error_prepend()
without ERRP_GUARD(), and though its @errp parameter points to its
caller's local_err, to follow the requirement of @errp, add missing
ERRP_GUARD() at the beginning of this function.

[1]: Issue description in the commit message of commit ae7c80a7bd73
 ("error: New macro ERRP_GUARD()").

Cc: Eric Blake 
Cc: Vladimir Sementsov-Ogievskiy 
Cc: Kevin Wolf 
Cc: Hanna Reitz 
Cc: qemu-block@nongnu.org
Signed-off-by: Zhao Liu 
Reviewed-by: Vladimir Sementsov-Ogievskiy 
---
 block/nbd.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/block/nbd.c b/block/nbd.c
index b9d4f935e017..ef05f7cdfd65 100644
--- a/block/nbd.c
+++ b/block/nbd.c
@@ -852,6 +852,7 @@ static coroutine_fn int nbd_co_do_receive_one_chunk(
 BDRVNBDState *s, uint64_t cookie, bool only_structured,
 int *request_ret, QEMUIOVector *qiov, void **payload, Error **errp)
 {
+ERRP_GUARD();
 int ret;
 int i = COOKIE_TO_INDEX(cookie);
 void *local_payload = NULL;
-- 
2.34.1

[PATCH v2 07/29] block/qcow2-bitmap: Fix missing ERRP_GUARD() for error_prepend()

[Zhao Liu]

From: Zhao Liu 

As the comment in qapi/error, passing @errp to error_prepend() requires
ERRP_GUARD():

* = Why, when and how to use ERRP_GUARD() =
*
* Without ERRP_GUARD(), use of the @errp parameter is restricted:
...
* - It should not be passed to error_prepend(), error_vprepend() or
*   error_append_hint(), because that doesn't work with _fatal.
* ERRP_GUARD() lifts these restrictions.
*
* To use ERRP_GUARD(), add it right at the beginning of the function.
* @errp can then be used without worrying about the argument being
* NULL or _fatal.

ERRP_GUARD() could avoid the case when @errp is _fatal, the user
can't see this additional information, because exit() happens in
error_setg earlier than information is added [1].

The qcow2_co_can_store_new_dirty_bitmap() passes @errp to
error_prepend(). As a BlockDriver.bdrv_co_can_store_new_dirty_bitmap
method, it's called by bdrv_co_can_store_new_dirty_bitmap().

Its caller is not being called anywhere, but as the API in
include/block/block-io.h, we can't ensure what kind of @errp future
users will pass in.

To avoid potential issues as [1] said, add missing ERRP_GUARD() at the
beginning of qcow2_co_can_store_new_dirty_bitmap().

[1]: Issue description in the commit message of commit ae7c80a7bd73
 ("error: New macro ERRP_GUARD()").

Cc: Kevin Wolf 
Cc: Hanna Reitz 
Cc: Eric Blake 
Cc: Vladimir Sementsov-Ogievskiy 
Cc: John Snow 
Cc: qemu-block@nongnu.org
Signed-off-by: Zhao Liu 
Reviewed-by: Vladimir Sementsov-Ogievskiy 
---
 block/qcow2-bitmap.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
index 0e567ed588d7..874ea5694851 100644
--- a/block/qcow2-bitmap.c
+++ b/block/qcow2-bitmap.c
@@ -1710,6 +1710,7 @@ bool coroutine_fn 
qcow2_co_can_store_new_dirty_bitmap(BlockDriverState *bs,
   uint32_t granularity,
   Error **errp)
 {
+ERRP_GUARD();
 BDRVQcow2State *s = bs->opaque;
 BdrvDirtyBitmap *bitmap;
 uint64_t bitmap_directory_size = 0;
-- 
2.34.1

[PATCH v2 11/29] block/vdi: Fix missing ERRP_GUARD() for error_prepend()

[Zhao Liu]

From: Zhao Liu 

As the comment in qapi/error, passing @errp to error_prepend() requires
ERRP_GUARD():

* = Why, when and how to use ERRP_GUARD() =
*
* Without ERRP_GUARD(), use of the @errp parameter is restricted:
...
* - It should not be passed to error_prepend(), error_vprepend() or
*   error_append_hint(), because that doesn't work with _fatal.
* ERRP_GUARD() lifts these restrictions.
*
* To use ERRP_GUARD(), add it right at the beginning of the function.
* @errp can then be used without worrying about the argument being
* NULL or _fatal.

ERRP_GUARD() could avoid the case when @errp is _fatal, the user
can't see this additional information, because exit() happens in
error_setg earlier than information is added [1].

The vdi_co_do_create() passes @errp to error_prepend() without
ERRP_GUARD(), and its @errp parameter is so widely sourced that it is
necessary to protect it with ERRP_GUARD().

To avoid the potential issues as [1] said, add missing ERRP_GUARD() at
the beginning of this function.

[1]: Issue description in the commit message of commit ae7c80a7bd73
 ("error: New macro ERRP_GUARD()").

Cc: Stefan Weil 
Cc: Kevin Wolf 
Cc: Hanna Reitz 
Cc: qemu-block@nongnu.org
Signed-off-by: Zhao Liu 
---
 block/vdi.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/block/vdi.c b/block/vdi.c
index 3b57becb9fe0..6363da08cee9 100644
--- a/block/vdi.c
+++ b/block/vdi.c
@@ -738,6 +738,7 @@ static int coroutine_fn GRAPH_UNLOCKED
 vdi_co_do_create(BlockdevCreateOptions *create_options, size_t block_size,
  Error **errp)
 {
+ERRP_GUARD();
 BlockdevCreateOptionsVdi *vdi_opts;
 int ret = 0;
 uint64_t bytes = 0;
-- 
2.34.1

[PATCH v2 03/29] block: Fix missing ERRP_GUARD() for error_prepend()

[Zhao Liu]

From: Zhao Liu 

As the comment in qapi/error, passing @errp to error_prepend() requires
ERRP_GUARD():

* = Why, when and how to use ERRP_GUARD() =
*
* Without ERRP_GUARD(), use of the @errp parameter is restricted:
...
* - It should not be passed to error_prepend(), error_vprepend() or
*   error_append_hint(), because that doesn't work with _fatal.
* ERRP_GUARD() lifts these restrictions.
*
* To use ERRP_GUARD(), add it right at the beginning of the function.
* @errp can then be used without worrying about the argument being
* NULL or _fatal.

ERRP_GUARD() could avoid the case when @errp is _fatal, the user
can't see this additional information, because exit() happens in
error_setg earlier than information is added [1].

In block.c, there are 4 functions passing @errp to error_prepend()
without ERRP_GUARD():
 - bdrv_co_create_opts_simple()
 - parse_json_filename()
 - bdrv_open_backing_file()
 - bdrv_append_temp_snapshot()

bdrv_co_create_opts_simple(), is an implementation of
BlockDriver.bdrv_co_create_opts(). There are too many possible callers
to check the impact of this defect; it may or may not be harmless. Thus
it is necessary to protect @errp with ERRP_GUARD().

Though the @errp parameters passed to parse_json_filename(),
bdrv_open_backing_file() and bdrv_append_temp_snapshot() points to their
callers' local_err, to follow the requirement of @errp, also add missing
ERRP_GUARD() at their beginning.

[1]: Issue description in the commit message of commit ae7c80a7bd73
 ("error: New macro ERRP_GUARD()").

Cc: Kevin Wolf 
Cc: Hanna Reitz 
Cc: qemu-block@nongnu.org
Signed-off-by: Zhao Liu 
Reviewed-by: Eric Blake 
---
v2:
 * Use Markus' sentence to polish commit message. (Markus)
 * Fix typo. (Eric)
---
 block.c | 4 
 1 file changed, 4 insertions(+)

diff --git a/block.c b/block.c
index 1ed9214f66ed..cf66c767a011 100644
--- a/block.c
+++ b/block.c
@@ -633,6 +633,7 @@ int coroutine_fn bdrv_co_create_opts_simple(BlockDriver 
*drv,
 QemuOpts *opts,
 Error **errp)
 {
+ERRP_GUARD();
 BlockBackend *blk;
 QDict *options;
 int64_t size = 0;
@@ -1998,6 +1999,7 @@ fail_opts:
 
 static QDict *parse_json_filename(const char *filename, Error **errp)
 {
+ERRP_GUARD();
 QObject *options_obj;
 QDict *options;
 int ret;
@@ -3585,6 +3587,7 @@ int bdrv_set_backing_hd(BlockDriverState *bs, 
BlockDriverState *backing_hd,
 int bdrv_open_backing_file(BlockDriverState *bs, QDict *parent_options,
const char *bdref_key, Error **errp)
 {
+ERRP_GUARD();
 char *backing_filename = NULL;
 char *bdref_key_dot;
 const char *reference = NULL;
@@ -3851,6 +3854,7 @@ static BlockDriverState 
*bdrv_append_temp_snapshot(BlockDriverState *bs,
QDict *snapshot_options,
Error **errp)
 {
+ERRP_GUARD();
 g_autofree char *tmp_filename = NULL;
 int64_t total_size;
 QemuOpts *opts = NULL;
-- 
2.34.1

Re: [PATCH 21/21] hw: Add QOM parentship relation with CPUs

[Zhao Liu]

On Fri, Feb 16, 2024 at 12:03:12PM +0100, Philippe Mathieu-Daudé wrote:
> Date: Fri, 16 Feb 2024 12:03:12 +0100
> From: Philippe Mathieu-Daudé 
> Subject: [PATCH 21/21] hw: Add QOM parentship relation with CPUs
> X-Mailer: git-send-email 2.41.0
> 
> QDev objects created with object_new() need to manually add
> their parent relationship with object_property_add_child().
> 
> Signed-off-by: Philippe Mathieu-Daudé 
> ---
>  hw/i386/x86.c| 1 +
>  hw/microblaze/petalogix_ml605_mmu.c  | 1 +
>  hw/microblaze/petalogix_s3adsp1800_mmu.c | 1 +
>  hw/mips/cps.c| 1 +
>  hw/nios2/10m50_devboard.c| 1 +
>  hw/ppc/e500.c| 1 +
>  hw/ppc/spapr.c   | 1 +
>  7 files changed, 7 insertions(+)

Reviewed-by: Zhao Liu 

> 
> diff --git a/hw/i386/x86.c b/hw/i386/x86.c
> index 684dce90e9..7021419d91 100644
> --- a/hw/i386/x86.c
> +++ b/hw/i386/x86.c
> @@ -102,6 +102,7 @@ void x86_cpu_new(X86MachineState *x86ms, int64_t apic_id, 
> Error **errp)
>  if (!object_property_set_uint(cpu, "apic-id", apic_id, errp)) {
>  goto out;
>  }
> +object_property_add_child(OBJECT(x86ms), "cpu[*]", OBJECT(cpu));
>  qdev_realize(DEVICE(cpu), NULL, errp);
>  
>  out:
> diff --git a/hw/microblaze/petalogix_ml605_mmu.c 
> b/hw/microblaze/petalogix_ml605_mmu.c
> index 0f5fabc32e..dfd881322d 100644
> --- a/hw/microblaze/petalogix_ml605_mmu.c
> +++ b/hw/microblaze/petalogix_ml605_mmu.c
> @@ -83,6 +83,7 @@ petalogix_ml605_init(MachineState *machine)
>  
>  /* init CPUs */
>  cpu = MICROBLAZE_CPU(object_new(TYPE_MICROBLAZE_CPU));
> +object_property_add_child(OBJECT(machine), "cpu", OBJECT(cpu));
>  object_property_set_str(OBJECT(cpu), "version", "8.10.a", _abort);
>  /* Use FPU but don't use floating point conversion and square
>   * root instructions
> diff --git a/hw/microblaze/petalogix_s3adsp1800_mmu.c 
> b/hw/microblaze/petalogix_s3adsp1800_mmu.c
> index dad46bd7f9..255d8d4d47 100644
> --- a/hw/microblaze/petalogix_s3adsp1800_mmu.c
> +++ b/hw/microblaze/petalogix_s3adsp1800_mmu.c
> @@ -70,6 +70,7 @@ petalogix_s3adsp1800_init(MachineState *machine)
>  MemoryRegion *sysmem = get_system_memory();
>  
>  cpu = MICROBLAZE_CPU(object_new(TYPE_MICROBLAZE_CPU));
> +object_property_add_child(OBJECT(machine), "cpu", OBJECT(cpu));
>  object_property_set_str(OBJECT(cpu), "version", "7.10.d", _abort);
>  qdev_realize(DEVICE(cpu), NULL, _abort);
>  
> diff --git a/hw/mips/cps.c b/hw/mips/cps.c
> index 07b73b0a1f..6b4e918807 100644
> --- a/hw/mips/cps.c
> +++ b/hw/mips/cps.c
> @@ -84,6 +84,7 @@ static void mips_cps_realize(DeviceState *dev, Error **errp)
>  /* All cores use the same clock tree */
>  qdev_connect_clock_in(DEVICE(cpu), "clk-in", s->clock);
>  
> +object_property_add_child(OBJECT(dev), "cpu[*]", OBJECT(cpu));
>  if (!qdev_realize_and_unref(DEVICE(cpu), NULL, errp)) {
>  return;
>  }
> diff --git a/hw/nios2/10m50_devboard.c b/hw/nios2/10m50_devboard.c
> index 6cb32f777b..f6a691d340 100644
> --- a/hw/nios2/10m50_devboard.c
> +++ b/hw/nios2/10m50_devboard.c
> @@ -95,6 +95,7 @@ static void nios2_10m50_ghrd_init(MachineState *machine)
>  cpu->exception_addr = 0xc8000120;
>  cpu->fast_tlb_miss_addr = 0xc100;
>  
> +object_property_add_child(OBJECT(machine), "cpu", OBJECT(cpu));
>  qdev_realize_and_unref(DEVICE(cpu), NULL, _fatal);
>  
>  if (nms->vic) {
> diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
> index 3bd12b54ab..77b7d2858c 100644
> --- a/hw/ppc/e500.c
> +++ b/hw/ppc/e500.c
> @@ -956,6 +956,7 @@ void ppce500_init(MachineState *machine)
>   */
>  object_property_set_bool(OBJECT(cs), "start-powered-off", i != 0,
>   _abort);
> +object_property_add_child(OBJECT(machine), "cpu[*]", OBJECT(cpu));
>  qdev_realize_and_unref(DEVICE(cs), NULL, _fatal);
>  
>  if (!firstenv) {
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 0d72d286d8..b6e5caa0d2 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -2715,6 +2715,7 @@ static void spapr_init_cpus(SpaprMachineState *spapr)
>  _fatal);
>  object_property_set_int(core, CPU_CORE_PROP_CORE_ID, core_id,
>  _fatal);
> +object_property_add_child(OBJECT(spapr), "cpu[*]", OBJECT(core));
>  qdev_realize(DEVICE(core), NULL, _fatal);
>  
>  object_unref(core);
> -- 
> 2.41.0
> 
> 

Re: [PATCH 03/21] hw/ppc/spapr_cpu: Use qdev_is_realized() instead of QOM API

[Zhao Liu]

On Fri, Feb 16, 2024 at 12:02:54PM +0100, Philippe Mathieu-Daudé wrote:
> Date: Fri, 16 Feb 2024 12:02:54 +0100
> From: Philippe Mathieu-Daudé 
> Subject: [PATCH 03/21] hw/ppc/spapr_cpu: Use qdev_is_realized() instead of
>  QOM API
> X-Mailer: git-send-email 2.41.0
> 
> Prefer QDev API for QDev objects, avoid the underlying QOM layer.
> 
> Signed-off-by: Philippe Mathieu-Daudé 
> ---
>  hw/ppc/spapr_cpu_core.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)

Reviewed-by: Zhao Liu 

> 
> diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> index 0c0fb3f1b0..40b7c52f7f 100644
> --- a/hw/ppc/spapr_cpu_core.c
> +++ b/hw/ppc/spapr_cpu_core.c
> @@ -245,8 +245,7 @@ static void spapr_cpu_core_unrealize(DeviceState *dev)
>   * spapr_cpu_core_realize(), make sure we only unrealize
>   * vCPUs that have already been realized.
>   */
> -if (object_property_get_bool(OBJECT(sc->threads[i]), "realized",
> - _abort)) {
> +if (qdev_is_realized(DEVICE(sc->threads[i]))) {
>  spapr_unrealize_vcpu(sc->threads[i], sc);
>  }
>  spapr_delete_vcpu(sc->threads[i]);
> -- 
> 2.41.0
> 
> 

Re: [PATCH 10/21] hw/usb: Inline usb_new()

[Zhao Liu]

On Fri, Feb 16, 2024 at 12:03:01PM +0100, Philippe Mathieu-Daudé wrote:
> Date: Fri, 16 Feb 2024 12:03:01 +0100
> From: Philippe Mathieu-Daudé 
> Subject: [PATCH 10/21] hw/usb: Inline usb_new()
> X-Mailer: git-send-email 2.41.0
> 
> Inline the 2 uses of usb_new().
> 
> Signed-off-by: Philippe Mathieu-Daudé 
> ---
>  include/hw/usb.h| 1 -
>  hw/usb/bus.c| 9 ++---
>  hw/usb/dev-serial.c | 2 +-
>  3 files changed, 3 insertions(+), 9 deletions(-)

Reviewed-by: Zhao Liu 

> 
> diff --git a/include/hw/usb.h b/include/hw/usb.h
> index 32c23a5ca2..2d820685cc 100644
> --- a/include/hw/usb.h
> +++ b/include/hw/usb.h
> @@ -500,7 +500,6 @@ void usb_bus_release(USBBus *bus);
>  USBBus *usb_bus_find(int busnr);
>  void usb_legacy_register(const char *typename, const char *usbdevice_name,
>   USBDevice *(*usbdevice_init)(void));
> -USBDevice *usb_new(const char *name);
>  bool usb_realize_and_unref(USBDevice *dev, USBBus *bus, Error **errp);
>  USBDevice *usb_create_simple(USBBus *bus, const char *name);
>  USBDevice *usbdevice_create(const char *cmdline);
> diff --git a/hw/usb/bus.c b/hw/usb/bus.c
> index 148224f06a..a599e2552b 100644
> --- a/hw/usb/bus.c
> +++ b/hw/usb/bus.c
> @@ -329,11 +329,6 @@ void usb_legacy_register(const char *typename, const 
> char *usbdevice_name,
>  }
>  }
>  
> -USBDevice *usb_new(const char *name)
> -{
> -return USB_DEVICE(qdev_new(name));
> -}
> -
>  bool usb_realize_and_unref(USBDevice *dev, USBBus *bus, Error **errp)
>  {
>  return qdev_realize_and_unref(>qdev, >qbus, errp);
> @@ -341,7 +336,7 @@ bool usb_realize_and_unref(USBDevice *dev, USBBus *bus, 
> Error **errp)
>  
>  USBDevice *usb_create_simple(USBBus *bus, const char *name)
>  {
> -USBDevice *dev = usb_new(name);
> +USBDevice *dev = USB_DEVICE(qdev_new(name));
>  
>  usb_realize_and_unref(dev, bus, _abort);
>  return dev;
> @@ -693,7 +688,7 @@ USBDevice *usbdevice_create(const char *driver)
>  return NULL;
>  }
>  
> -dev = f->usbdevice_init ? f->usbdevice_init() : usb_new(f->name);
> +dev = f->usbdevice_init ? f->usbdevice_init() : 
> USB_DEVICE(qdev_new(f->name));
>  if (!dev) {
>  error_report("Failed to create USB device '%s'", f->name);
>  return NULL;
> diff --git a/hw/usb/dev-serial.c b/hw/usb/dev-serial.c
> index 63047d79cf..6e79c46d53 100644
> --- a/hw/usb/dev-serial.c
> +++ b/hw/usb/dev-serial.c
> @@ -624,7 +624,7 @@ static USBDevice *usb_braille_init(void)
>  return NULL;
>  }
>  
> -dev = usb_new("usb-braille");
> +dev = USB_DEVICE(qdev_new("usb-braille"));
>  qdev_prop_set_chr(>qdev, "chardev", cdrv);
>  return dev;
>  }
> -- 
> 2.41.0
> 
> 

Re: [PATCH 07/21] target: Replace DEVICE(object_new) -> qdev_new()

[Zhao Liu]

On Fri, Feb 16, 2024 at 12:02:58PM +0100, Philippe Mathieu-Daudé wrote:
> Date: Fri, 16 Feb 2024 12:02:58 +0100
> From: Philippe Mathieu-Daudé 
> Subject: [PATCH 07/21] target: Replace DEVICE(object_new) -> qdev_new()
> X-Mailer: git-send-email 2.41.0
> 
> Prefer QDev API for QDev objects, avoid the underlying QOM layer.
> 
> Signed-off-by: Philippe Mathieu-Daudé 
> ---
>  target/mips/cpu.c   | 2 +-
>  target/xtensa/cpu.c | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)

Reviewed-by: Zhao Liu 

> 
> diff --git a/target/mips/cpu.c b/target/mips/cpu.c
> index d644adbc77..6b3909ee08 100644
> --- a/target/mips/cpu.c
> +++ b/target/mips/cpu.c
> @@ -649,7 +649,7 @@ MIPSCPU *mips_cpu_create_with_clock(const char *cpu_type, 
> Clock *cpu_refclk)
>  {
>  DeviceState *cpu;
>  
> -cpu = DEVICE(object_new(cpu_type));
> +cpu = qdev_new(cpu_type);
>  qdev_connect_clock_in(cpu, "clk-in", cpu_refclk);
>  qdev_realize(cpu, NULL, _abort);
>  
> diff --git a/target/xtensa/cpu.c b/target/xtensa/cpu.c
> index 79f91819df..4f9408e1a0 100644
> --- a/target/xtensa/cpu.c
> +++ b/target/xtensa/cpu.c
> @@ -205,7 +205,7 @@ XtensaCPU *xtensa_cpu_create_with_clock(const char 
> *cpu_type, Clock *cpu_refclk)
>  {
>  DeviceState *cpu;
>  
> -cpu = DEVICE(object_new(cpu_type));
> +cpu = qdev_new(cpu_type);
>  qdev_connect_clock_in(cpu, "clk-in", cpu_refclk);
>  qdev_realize(cpu, NULL, _abort);
>  
> -- 
> 2.41.0
> 
> 

Re: [PATCH 09/21] hw/usb: Inline usb_try_new()

[Zhao Liu]

On Fri, Feb 16, 2024 at 12:03:00PM +0100, Philippe Mathieu-Daudé wrote:
> Date: Fri, 16 Feb 2024 12:03:00 +0100
> From: Philippe Mathieu-Daudé 
> Subject: [PATCH 09/21] hw/usb: Inline usb_try_new()
> X-Mailer: git-send-email 2.41.0
> 
> Inline the single use of usb_try_new().
> 
> Signed-off-by: Philippe Mathieu-Daudé 
> ---
>  hw/usb/bus.c | 7 +--
>  1 file changed, 1 insertion(+), 6 deletions(-)

Reviewed-by: Zhao Liu 

> 
> diff --git a/hw/usb/bus.c b/hw/usb/bus.c
> index 59c39945dd..148224f06a 100644
> --- a/hw/usb/bus.c
> +++ b/hw/usb/bus.c
> @@ -334,11 +334,6 @@ USBDevice *usb_new(const char *name)
>  return USB_DEVICE(qdev_new(name));
>  }
>  
> -static USBDevice *usb_try_new(const char *name)
> -{
> -return USB_DEVICE(qdev_try_new(name));
> -}
> -
>  bool usb_realize_and_unref(USBDevice *dev, USBBus *bus, Error **errp)
>  {
>  return qdev_realize_and_unref(>qdev, >qbus, errp);
> @@ -447,7 +442,7 @@ void usb_claim_port(USBDevice *dev, Error **errp)
>  } else {
>  if (bus->nfree == 1 && strcmp(object_get_typename(OBJECT(dev)), 
> "usb-hub") != 0) {
>  /* Create a new hub and chain it on */
> -hub = usb_try_new("usb-hub");
> +hub = USB_DEVICE(qdev_try_new("usb-hub"));
>  if (hub) {
>  usb_realize_and_unref(hub, bus, NULL);
>  }
> -- 
> 2.41.0
> 
> 

Re: [PATCH 06/21] hw: Replace DEVICE(object_new) -> qdev_new()

[Zhao Liu]

On Fri, Feb 16, 2024 at 12:02:57PM +0100, Philippe Mathieu-Daudé wrote:
> Date: Fri, 16 Feb 2024 12:02:57 +0100
> From: Philippe Mathieu-Daudé 
> Subject: [PATCH 06/21] hw: Replace DEVICE(object_new) -> qdev_new()
> X-Mailer: git-send-email 2.41.0
> 
> Prefer QDev API for QDev objects, avoid the underlying QOM layer.
> 
> Signed-off-by: Philippe Mathieu-Daudé 
> ---
>  hw/arm/musicpal.c | 2 +-
>  hw/core/qdev.c| 2 +-
>  hw/sparc/sun4m.c  | 4 ++--
>  3 files changed, 4 insertions(+), 4 deletions(-)

Reviewed-by: Zhao Liu 

> 
> diff --git a/hw/arm/musicpal.c b/hw/arm/musicpal.c
> index 2020f73a57..74e4d24aab 100644
> --- a/hw/arm/musicpal.c
> +++ b/hw/arm/musicpal.c
> @@ -1238,7 +1238,7 @@ static void musicpal_init(MachineState *machine)
>qdev_get_gpio_in(pic, MP_TIMER4_IRQ), NULL);
>  
>  /* Logically OR both UART IRQs together */
> -uart_orgate = DEVICE(object_new(TYPE_OR_IRQ));
> +uart_orgate = qdev_new(TYPE_OR_IRQ);
>  object_property_set_int(OBJECT(uart_orgate), "num-lines", 2, 
> _fatal);
>  qdev_realize_and_unref(uart_orgate, NULL, _fatal);
>  qdev_connect_gpio_out(uart_orgate, 0,
> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> index c68d0f7c51..a271380d20 100644
> --- a/hw/core/qdev.c
> +++ b/hw/core/qdev.c
> @@ -171,7 +171,7 @@ DeviceState *qdev_try_new(const char *name)
>  if (!module_object_class_by_name(name)) {
>  return NULL;
>  }
> -return DEVICE(object_new(name));
> +return qdev_new(name);
>  }
>  
>  static QTAILQ_HEAD(, DeviceListener) device_listeners
> diff --git a/hw/sparc/sun4m.c b/hw/sparc/sun4m.c
> index d52e6a7213..fedc4b8b3f 100644
> --- a/hw/sparc/sun4m.c
> +++ b/hw/sparc/sun4m.c
> @@ -979,7 +979,7 @@ static void sun4m_hw_init(MachineState *machine)
>  sysbus_mmio_map(s, 0, hwdef->ms_kb_base);
>  
>  /* Logically OR both its IRQs together */
> -ms_kb_orgate = DEVICE(object_new(TYPE_OR_IRQ));
> +ms_kb_orgate = qdev_new(TYPE_OR_IRQ);
>  object_property_set_int(OBJECT(ms_kb_orgate), "num-lines", 2, 
> _fatal);
>  qdev_realize_and_unref(ms_kb_orgate, NULL, _fatal);
>  sysbus_connect_irq(s, 0, qdev_get_gpio_in(ms_kb_orgate, 0));
> @@ -1000,7 +1000,7 @@ static void sun4m_hw_init(MachineState *machine)
>  sysbus_mmio_map(s, 0, hwdef->serial_base);
>  
>  /* Logically OR both its IRQs together */
> -serial_orgate = DEVICE(object_new(TYPE_OR_IRQ));
> +serial_orgate = qdev_new(TYPE_OR_IRQ);
>  object_property_set_int(OBJECT(serial_orgate), "num-lines", 2,
>  _fatal);
>  qdev_realize_and_unref(serial_orgate, NULL, _fatal);
> -- 
> 2.41.0
> 
> 

Re: [PATCH 11/21] hw/usb: Add QOM parentship relation with hub devices

[Zhao Liu]

Hi Philippe,

On Fri, Feb 16, 2024 at 12:03:02PM +0100, Philippe Mathieu-Daudé wrote:
> Date: Fri, 16 Feb 2024 12:03:02 +0100
> From: Philippe Mathieu-Daudé 
> Subject: [PATCH 11/21] hw/usb: Add QOM parentship relation with hub devices
> X-Mailer: git-send-email 2.41.0
> 
> QDev objects created with qdev_*new() need to manually add
> their parent relationship with object_property_add_child().
> 
> Signed-off-by: Philippe Mathieu-Daudé 
> ---
>  hw/usb/bus.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/hw/usb/bus.c b/hw/usb/bus.c
> index a599e2552b..baad04f466 100644
> --- a/hw/usb/bus.c
> +++ b/hw/usb/bus.c
> @@ -439,6 +439,7 @@ void usb_claim_port(USBDevice *dev, Error **errp)
>  /* Create a new hub and chain it on */
>  hub = USB_DEVICE(qdev_try_new("usb-hub"));

One additional question comes to mind, should we use the qdev_new()
here?

The difference between qdev_try_new() and qdev_new() is the latter
uses assert() to ensure the passed type exists.

So if we know that type parameter is correct, we should just use
qdev_new().

Only when the caller is not sure whether the type is valid,
qdev_try_new() should be preferred.

Am I understand correctly? ;-)

>  if (hub) {
> +object_property_add_child(OBJECT(dev), "hub", OBJECT(hub));

>From the comment above the code:

/* Create a new hub and chain it on */

this only creates a new usb-hub, should the new usb-hub become the child
object of the original usb-hub?

>  usb_realize_and_unref(hub, bus, NULL);
>  }
>  }
> -- 
> 2.41.0
> 
> 

Re: [PATCH 17/21] hw/i386/iommu: Prefer object_initialize_child over object_initialize

[Zhao Liu]

Hi Philippe,

On Fri, Feb 16, 2024 at 12:03:08PM +0100, Philippe Mathieu-Daudé wrote:
> Date: Fri, 16 Feb 2024 12:03:08 +0100
> From: Philippe Mathieu-Daudé 
> Subject: [PATCH 17/21] hw/i386/iommu: Prefer object_initialize_child over
>  object_initialize
> X-Mailer: git-send-email 2.41.0
> 
> When the QOM parent is available, prefer object_initialize_child()
> over object_initialize(), since it create the parent relationship.
> 
> Rename the 'klass' variable as 'obj' since the argument holds a
> reference to an instance object and not a class one.
> 
> Signed-off-by: Philippe Mathieu-Daudé 
> ---
>  hw/i386/amd_iommu.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/i386/amd_iommu.c b/hw/i386/amd_iommu.c
> index 7329553ad3..c3afbc4130 100644
> --- a/hw/i386/amd_iommu.c
> +++ b/hw/i386/amd_iommu.c
> @@ -1616,11 +1616,11 @@ static const VMStateDescription vmstate_amdvi_sysbus 
> = {
>  .unmigratable = 1
>  };
>  
> -static void amdvi_sysbus_instance_init(Object *klass)
> +static void amdvi_sysbus_instance_init(Object *obj)
>  {
> -AMDVIState *s = AMD_IOMMU_DEVICE(klass);
> +AMDVIState *s = AMD_IOMMU_DEVICE(obj);
>  
> -object_initialize(>pci, sizeof(s->pci), TYPE_AMD_IOMMU_PCI);
> +object_initialize_child(obj, "iommu", >pci, TYPE_AMD_IOMMU_PCI);

What about this name "amd-iommu"?

This is more accurate and differentiates it from the other intel-iommu
related implementations.

>  }
>  
>  static void amdvi_sysbus_class_init(ObjectClass *klass, void *data)
> -- 
> 2.41.0
> 
> 

Re: [PATCH 02/21] hw/i386/pc_sysfw: Use qdev_is_realized() instead of QOM API

[Zhao Liu]

On Fri, Feb 16, 2024 at 12:02:53PM +0100, Philippe Mathieu-Daudé wrote:
> Date: Fri, 16 Feb 2024 12:02:53 +0100
> From: Philippe Mathieu-Daudé 
> Subject: [PATCH 02/21] hw/i386/pc_sysfw: Use qdev_is_realized() instead of
>  QOM API
> X-Mailer: git-send-email 2.41.0
> 
> Prefer QDev API for QDev objects, avoid the underlying QOM layer.
> 
> Signed-off-by: Philippe Mathieu-Daudé 
> ---
>  hw/i386/pc_sysfw.c | 6 ++
>  1 file changed, 2 insertions(+), 4 deletions(-)

Reviewed-by: Zhao Liu 

> 
> diff --git a/hw/i386/pc_sysfw.c b/hw/i386/pc_sysfw.c
> index c8d9e71b88..3efabbbab2 100644
> --- a/hw/i386/pc_sysfw.c
> +++ b/hw/i386/pc_sysfw.c
> @@ -107,17 +107,15 @@ void pc_system_flash_cleanup_unused(PCMachineState 
> *pcms)
>  {
>  char *prop_name;
>  int i;
> -Object *dev_obj;
>  
>  assert(PC_MACHINE_GET_CLASS(pcms)->pci_enabled);
>  
>  for (i = 0; i < ARRAY_SIZE(pcms->flash); i++) {
> -dev_obj = OBJECT(pcms->flash[i]);
> -if (!object_property_get_bool(dev_obj, "realized", _abort)) {
> +if (!qdev_is_realized(DEVICE(pcms->flash[i]))) {
>  prop_name = g_strdup_printf("pflash%d", i);
>  object_property_del(OBJECT(pcms), prop_name);
>  g_free(prop_name);
> -object_unparent(dev_obj);
> +object_unparent(OBJECT(pcms->flash[i]));
>  pcms->flash[i] = NULL;
>  }
>  }
> -- 
> 2.41.0
> 
> 

Re: [PATCH 01/21] hw/i386/pc: Do not use C99 mixed-declarations style

[Zhao Liu]

Hi Philippe,

On Fri, Feb 16, 2024 at 12:02:52PM +0100, Philippe Mathieu-Daudé wrote:
> Date: Fri, 16 Feb 2024 12:02:52 +0100
> From: Philippe Mathieu-Daudé 
> Subject: [PATCH 01/21] hw/i386/pc: Do not use C99 mixed-declarations style
> X-Mailer: git-send-email 2.41.0
> 
> QEMU's coding style generally forbids C99 mixed declarations.
> 
> Signed-off-by: Philippe Mathieu-Daudé 
> ---
>  hw/i386/pc.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index 196827531a..3c00a87317 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -1227,6 +1227,7 @@ void pc_basic_device_init(struct PCMachineState *pcms,
>   */
>  if (pcms->hpet_enabled) {
>  qemu_irq rtc_irq;
> +uint8_t compat;
>  
>  hpet = qdev_try_new(TYPE_HPET);
>  if (!hpet) {
> @@ -1238,8 +1239,7 @@ void pc_basic_device_init(struct PCMachineState *pcms,
>   * use IRQ16~23, IRQ8 and IRQ2.  If the user has already set
>   * the property, use whatever mask they specified.
>   */
> -uint8_t compat = object_property_get_uint(OBJECT(hpet),
> -HPET_INTCAP, NULL);
> +compat = object_property_get_uint(OBJECT(hpet), HPET_INTCAP, NULL);
>  if (!compat) {
>  qdev_prop_set_uint32(hpet, HPET_INTCAP, hpet_irqs);
>  }

"compat" is only used here to check. So, what about getting rid of this
variable?

if (!object_property_get_uint(OBJECT(hpet), HPET_INTCAP, NULL)) {
qdev_prop_set_uint32(hpet, HPET_INTCAP, hpet_irqs);
}

Re: [PATCH] docs: fix typo

[Zhao Liu]

On Thu, Dec 14, 2023 at 11:53:18PM +0100, Samuel Tardieu wrote:
> Date: Thu, 14 Dec 2023 23:53:18 +0100
> From: Samuel Tardieu 
> Subject: [PATCH] docs: fix typo
> X-Mailer: git-send-email 2.42.0
> 
> Signed-off-by: Samuel Tardieu 
> ---

Reviewed-by: Zhao Liu 

>  docs/tools/qemu-img.rst | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/docs/tools/qemu-img.rst b/docs/tools/qemu-img.rst
> index 4459c065f1..3653adb963 100644
> --- a/docs/tools/qemu-img.rst
> +++ b/docs/tools/qemu-img.rst
> @@ -406,7 +406,7 @@ Command description:
>Compare exits with ``0`` in case the images are equal and with ``1``
>in case the images differ. Other exit codes mean an error occurred during
>execution and standard error output should contain an error message.
> -  The following table sumarizes all exit codes of the compare subcommand:
> +  The following table summarizes all exit codes of the compare subcommand:
>  
>0
>  Images are identical (or requested help was printed)
> -- 
> 2.42.0
> 
>