diff mbox series

[PULL,13/29] qapi: Flatten object-add

Message ID 20200306171458.1848-14-kwolf@redhat.com
State New
Headers show
Series [PULL,01/29] qcow2: Fix alloc_cluster_abort() for pre-existing clusters | expand

Commit Message

Kevin Wolf March 6, 2020, 5:14 p.m. UTC
Mapping object-add to the command line as is doesn't result in nice
syntax because of the nesting introduced with 'props'. This becomes
nicer and more consistent with device_add and netdev_add when we accept
properties for the object on the top level instead.

'props' is still accepted after this patch, but marked as deprecated.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Message-Id: <20200224143008.13362-8-kwolf@redhat.com>
Acked-by: Stefan Hajnoczi <stefanha@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 qapi/qom.json                   | 12 +++++++---
 docs/system/deprecated.rst      |  5 ++++
 include/qom/object_interfaces.h |  7 ++++++
 hw/block/xen-block.c            | 11 ++++++++-
 monitor/misc.c                  |  2 ++
 qom/qom-qmp-cmds.c              | 42 +++++++++++++++++++++++++++------
 6 files changed, 68 insertions(+), 11 deletions(-)

Comments

Paolo Bonzini July 8, 2020, 3:48 p.m. UTC | #1
On 06/03/20 18:14, Kevin Wolf wrote:
> Mapping object-add to the command line as is doesn't result in nice
> syntax because of the nesting introduced with 'props'. This becomes
> nicer and more consistent with device_add and netdev_add when we accept
> properties for the object on the top level instead.
> 
> 'props' is still accepted after this patch, but marked as deprecated.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> Message-Id: <20200224143008.13362-8-kwolf@redhat.com>
> Acked-by: Stefan Hajnoczi <stefanha@redhat.com>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>

Hi Kevin and Markus,

I just noticed this patch.  In my opinion "nice syntax" is not a good
argument for having a "gen: false" command that is even less
introspectable than the "props" argument.

As an aside, it would have been nice to run this through Markus and me,
though in all fairness I'm not sure I would have been responsive back in
February.

I would like to un-deprecate this for 5.1, and revert it in either 5.1
or 5.2.  (Also I will be away next week, so the decision would have to
be taken quickly).

Paolo


> ---
>  qapi/qom.json                   | 12 +++++++---
>  docs/system/deprecated.rst      |  5 ++++
>  include/qom/object_interfaces.h |  7 ++++++
>  hw/block/xen-block.c            | 11 ++++++++-
>  monitor/misc.c                  |  2 ++
>  qom/qom-qmp-cmds.c              | 42 +++++++++++++++++++++++++++------
>  6 files changed, 68 insertions(+), 11 deletions(-)
> 
> diff --git a/qapi/qom.json b/qapi/qom.json
> index ecc60c4401..8abe998962 100644
> --- a/qapi/qom.json
> +++ b/qapi/qom.json
> @@ -210,7 +210,12 @@
>  #
>  # @id: the name of the new object
>  #
> -# @props: a dictionary of properties to be passed to the backend
> +# @props: a dictionary of properties to be passed to the backend. Deprecated
> +#         since 5.0, specify the properties on the top level instead. It is an
> +#         error to specify the same option both on the top level and in @props.
> +#
> +# Additional arguments depend on qom-type and are passed to the backend
> +# unchanged.
>  #
>  # Returns: Nothing on success
>  #          Error if @qom-type is not a valid class name
> @@ -221,12 +226,13 @@
>  #
>  # -> { "execute": "object-add",
>  #      "arguments": { "qom-type": "rng-random", "id": "rng1",
> -#                     "props": { "filename": "/dev/hwrng" } } }
> +#                     "filename": "/dev/hwrng" } }
>  # <- { "return": {} }
>  #
>  ##
>  { 'command': 'object-add',
> -  'data': {'qom-type': 'str', 'id': 'str', '*props': 'any'} }
> +  'data': {'qom-type': 'str', 'id': 'str', '*props': 'any'},
> +  'gen': false } # so we can get the additional arguments
>  
>  ##
>  # @object-del:
> diff --git a/docs/system/deprecated.rst b/docs/system/deprecated.rst
> index 1eaa559079..6c1d9034d9 100644
> --- a/docs/system/deprecated.rst
> +++ b/docs/system/deprecated.rst
> @@ -190,6 +190,11 @@ Use ``migrate-set-parameters`` instead.
>  
>  Use ``migrate-set-parameters`` and ``query-migrate-parameters`` instead.
>  
> +``object-add`` option ``props`` (since 5.0)
> +'''''''''''''''''''''''''''''''''''''''''''
> +
> +Specify the properties for the object as top-level arguments instead.
> +
>  ``query-block`` result field ``dirty-bitmaps[i].status`` (since 4.0)
>  ''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''
>  
> diff --git a/include/qom/object_interfaces.h b/include/qom/object_interfaces.h
> index 3e4e1d928b..6f92f3cebb 100644
> --- a/include/qom/object_interfaces.h
> +++ b/include/qom/object_interfaces.h
> @@ -162,4 +162,11 @@ void user_creatable_del(const char *id, Error **errp);
>   */
>  void user_creatable_cleanup(void);
>  
> +/**
> + * qmp_object_add:
> + *
> + * QMP command handler for object-add. See the QAPI schema for documentation.
> + */
> +void qmp_object_add(QDict *qdict, QObject **ret_data, Error **errp);
> +
>  #endif
> diff --git a/hw/block/xen-block.c b/hw/block/xen-block.c
> index 686bbc3f0d..3885464513 100644
> --- a/hw/block/xen-block.c
> +++ b/hw/block/xen-block.c
> @@ -18,6 +18,7 @@
>  #include "qapi/visitor.h"
>  #include "qapi/qmp/qdict.h"
>  #include "qapi/qmp/qstring.h"
> +#include "qom/object_interfaces.h"
>  #include "hw/xen/xen_common.h"
>  #include "hw/block/xen_blkif.h"
>  #include "hw/qdev-properties.h"
> @@ -858,10 +859,18 @@ static XenBlockIOThread *xen_block_iothread_create(const char *id,
>  {
>      XenBlockIOThread *iothread = g_new(XenBlockIOThread, 1);
>      Error *local_err = NULL;
> +    QDict *opts;
> +    QObject *ret_data;
>  
>      iothread->id = g_strdup(id);
>  
> -    qmp_object_add(TYPE_IOTHREAD, id, false, NULL, &local_err);
> +    opts = qdict_new();
> +    qdict_put_str(opts, "qom-type", TYPE_IOTHREAD);
> +    qdict_put_str(opts, "id", id);
> +    qmp_object_add(opts, &ret_data, &local_err);
> +    qobject_unref(opts);
> +    qobject_unref(ret_data);
> +
>      if (local_err) {
>          error_propagate(errp, local_err);
>  
> diff --git a/monitor/misc.c b/monitor/misc.c
> index 6c41293102..1748ab3911 100644
> --- a/monitor/misc.c
> +++ b/monitor/misc.c
> @@ -248,6 +248,8 @@ static void monitor_init_qmp_commands(void)
>                           QCO_NO_OPTIONS);
>      qmp_register_command(&qmp_commands, "netdev_add", qmp_netdev_add,
>                           QCO_NO_OPTIONS);
> +    qmp_register_command(&qmp_commands, "object-add", qmp_object_add,
> +                         QCO_NO_OPTIONS);
>  
>      QTAILQ_INIT(&qmp_cap_negotiation_commands);
>      qmp_register_command(&qmp_cap_negotiation_commands, "qmp_capabilities",
> diff --git a/qom/qom-qmp-cmds.c b/qom/qom-qmp-cmds.c
> index 6136efec16..49db926fcc 100644
> --- a/qom/qom-qmp-cmds.c
> +++ b/qom/qom-qmp-cmds.c
> @@ -14,6 +14,7 @@
>   */
>  
>  #include "qemu/osdep.h"
> +#include "block/qdict.h"
>  #include "hw/qdev-core.h"
>  #include "qapi/error.h"
>  #include "qapi/qapi-commands-qdev.h"
> @@ -240,13 +241,34 @@ ObjectPropertyInfoList *qmp_qom_list_properties(const char *typename,
>      return prop_list;
>  }
>  
> -void qmp_object_add(const char *type, const char *id,
> -                    bool has_props, QObject *props, Error **errp)
> +void qmp_object_add(QDict *qdict, QObject **ret_data, Error **errp)
>  {
> +    QObject *props;
>      QDict *pdict;
>      Visitor *v;
>      Object *obj;
> +    const char *type;
> +    const char *id;
>  
> +    type = qdict_get_try_str(qdict, "qom-type");
> +    if (!type) {
> +        error_setg(errp, QERR_MISSING_PARAMETER, "qom-type");
> +        return;
> +    } else {
> +        type = g_strdup(type);
> +        qdict_del(qdict, "qom-type");
> +    }
> +
> +    id = qdict_get_try_str(qdict, "id");
> +    if (!id) {
> +        error_setg(errp, QERR_MISSING_PARAMETER, "id");
> +        return;
> +    } else {
> +        id = g_strdup(id);
> +        qdict_del(qdict, "id");
> +    }
> +
> +    props = qdict_get(qdict, "props");
>      if (props) {
>          pdict = qobject_to(QDict, props);
>          if (!pdict) {
> @@ -254,17 +276,23 @@ void qmp_object_add(const char *type, const char *id,
>              return;
>          }
>          qobject_ref(pdict);
> -    } else {
> -        pdict = qdict_new();
> +        qdict_del(qdict, "props");
> +        qdict_join(qdict, pdict, false);
> +        if (qdict_size(pdict) != 0) {
> +            error_setg(errp, "Option in 'props' conflicts with top level");
> +            qobject_unref(pdict);
> +            return;
> +        }
> +        qobject_unref(pdict);
>      }
>  
> -    v = qobject_input_visitor_new(QOBJECT(pdict));
> -    obj = user_creatable_add_type(type, id, pdict, v, errp);
> +    v = qobject_input_visitor_new(QOBJECT(qdict));
> +    obj = user_creatable_add_type(type, id, qdict, v, errp);
>      visit_free(v);
>      if (obj) {
>          object_unref(obj);
>      }
> -    qobject_unref(pdict);
> +    *ret_data = QOBJECT(qdict_new());
>  }
>  
>  void qmp_object_del(const char *id, Error **errp)
>
Kevin Wolf July 8, 2020, 4:05 p.m. UTC | #2
Am 08.07.2020 um 17:48 hat Paolo Bonzini geschrieben:
> On 06/03/20 18:14, Kevin Wolf wrote:
> > Mapping object-add to the command line as is doesn't result in nice
> > syntax because of the nesting introduced with 'props'. This becomes
> > nicer and more consistent with device_add and netdev_add when we accept
> > properties for the object on the top level instead.
> > 
> > 'props' is still accepted after this patch, but marked as deprecated.
> > 
> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > Message-Id: <20200224143008.13362-8-kwolf@redhat.com>
> > Acked-by: Stefan Hajnoczi <stefanha@redhat.com>
> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> 
> Hi Kevin and Markus,
> 
> I just noticed this patch.  In my opinion "nice syntax" is not a good
> argument for having a "gen: false" command that is even less
> introspectable than the "props" argument.

Markus was going to introduce new QAPI schema syntax that would allow to
specify a few options explicitly and then one option for "the rest" that
would just be mapped to a QDict like "any" or "gen": false, and that
wouldn't require any nesting.

I'm not sure if any progress was made there, but making things
consistent between device_add, netdev_add and object_add was a step in
this direction anyway.

> As an aside, it would have been nice to run this through Markus and me,
> though in all fairness I'm not sure I would have been responsive back
> in February.

It went through my tree because of the other patches in the series, but
I wrote this patch specifically at Markus's request.

> I would like to un-deprecate this for 5.1, and revert it in either 5.1
> or 5.2.  (Also I will be away next week, so the decision would have to
> be taken quickly).

Please discuss it with Markus then.

Kevin
Paolo Bonzini July 8, 2020, 4:12 p.m. UTC | #3
On 08/07/20 18:05, Kevin Wolf wrote:
> Markus was going to introduce new QAPI schema syntax that would allow to
> specify a few options explicitly and then one option for "the rest" that
> would just be mapped to a QDict like "any" or "gen": false, and that
> wouldn't require any nesting.

Oh, I wasn't aware of that.  That would be something like 'properties':
'remainder' I guess.  That would be fine too.

Paolo

> I'm not sure if any progress was made there, but making things
> consistent between device_add, netdev_add and object_add was a step in
> this direction anyway.
> 
>> As an aside, it would have been nice to run this through Markus and me,
>> though in all fairness I'm not sure I would have been responsive back
>> in February.
> It went through my tree because of the other patches in the series, but
> I wrote this patch specifically at Markus's request.
> 
>> I would like to un-deprecate this for 5.1, and revert it in either 5.1
>> or 5.2.  (Also I will be away next week, so the decision would have to
>> be taken quickly).
> Please discuss it with Markus then.
Markus Armbruster July 9, 2020, 10:26 a.m. UTC | #4
Paolo Bonzini <pbonzini@redhat.com> writes:

> On 08/07/20 18:05, Kevin Wolf wrote:
>> Markus was going to introduce new QAPI schema syntax that would allow to
>> specify a few options explicitly and then one option for "the rest" that
>> would just be mapped to a QDict like "any" or "gen": false, and that
>> wouldn't require any nesting.
>
> Oh, I wasn't aware of that.  That would be something like 'properties':
> 'remainder' I guess.  That would be fine too.

Glad I'm spared a design argument ;)

>> I'm not sure if any progress was made there, but making things

Not yet; error handling ate me whole, and spit me out only the day
before yesterday or so.

>> consistent between device_add, netdev_add and object_add was a step in
>> this direction anyway.

Yes.

Permit me to digress.

We ran into the "and also arbitray properties" need repeatedly, and
tried several solutions over time.  QAPI/QMP is big, and the left hand
often wasn't too interested in what the right hand had been doing.

If memory and my git archaeology skills serve, the first instance was
device_add:

    3418bd25e1 "qdev hotplug: infrastructure and monitor commands."
    2009-10-05

Simple adaption of the QemuOpts-based -device for HMP.  Since QemuOpts
is flat, so is device_add.  QAPI didn't exist, yet.  A QMP version
followed:

    8bc27249f0 "monitor: convert do_device_add() to QObject"
    2010-03-16

Just as flat.

Next was netdev_add:

    ae82d3242d "monitor: New commands netdev_add, netdev_del"
    2010-04-18

Flat for the same reason.

When QAPI came along in

    ebffe2afce "Merge remote-tracking branch 'qmp/queue/qmp' into
    staging"
    2011-10-10

these two commands were left unQAPIfied, as the schema language could
not express "and also arbitrary properties".  Soon "solved" with a cop
out:

    5dbee474f3 "qapi: allow a 'gen' key to suppress code generation"
    2011-12-15

A half-hearted QAPIfication of netdev_add followed:

    928059a37b "qapi: convert netdev_add"
    2012-06-04

QAPI schema:

    { 'command': 'netdev_add',
      'data': {'type': 'str', 'id': 'str', '*props': '**'},
      'gen': 'no' }

Note the bogus type '**', which only works because 'gen': 'no' also
bypasses type checking (which you wouldn't guess from the commit
message, documentation, or even the schema).  In fact, the whole 'props'
thing is a lie: there is no such parameter, the command is as flat as it
ever was.  Fixed in

    b8a98326d5 "qapi-schema: Fix up misleading specification of
    netdev_add"
    2015-09-21

    { 'command': 'netdev_add',
      'data': {'type': 'str', 'id': 'str'},
      'gen': false }

but by then it had already spread to object-add, with an equally bogus
type 'dict':

    cff8b2c6fc "monitor: add object-add (QMP) and object_add (HMP)
    command"
    2014-01-06

    { 'command': 'object-add',
      'data': {'qom-type': 'str', 'id': 'str', '*props': 'dict'},
      'gen': 'no' }

Only 'props' was real this time: you really had to wrap the properties
in "props": { ... }.  Non-flat.  Meh.

Eventually, even device_add made it into the schema:

    94cfd07f26 "qapi-schema: add 'device_add'"
    2016-09-19

    { 'command': 'device_add',
      'data': {'driver': 'str', 'id': 'str'},
      'gen': false } # so we can get the additional arguments

And netdev_add was finally done right:

    db2a380c84 "net: Complete qapi-fication of netdev_add"
    2020-03-17

    { 'command': 'netdev_add', 'data': 'Netdev', 'boxed': true }

Doing device_add and object-add right is harder (impractical?), because
their schema would be a union with one branch per device / object type.

End of digression.

>>> As an aside, it would have been nice to run this through Markus and me,
>>> though in all fairness I'm not sure I would have been responsive back
>>> in February.
>> It went through my tree because of the other patches in the series, but
>> I wrote this patch specifically at Markus's request.

Yes.  We discussed how to best satisfy Kevin's immediate needs without
making other problems harder.  Perhaps we should have posted a summary
to the list.

>>> I would like to un-deprecate this for 5.1, and revert it in either 5.1
>>> or 5.2.  (Also I will be away next week, so the decision would have to
>>> be taken quickly).
>> Please discuss it with Markus then.
diff mbox series

Patch

diff --git a/qapi/qom.json b/qapi/qom.json
index ecc60c4401..8abe998962 100644
--- a/qapi/qom.json
+++ b/qapi/qom.json
@@ -210,7 +210,12 @@ 
 #
 # @id: the name of the new object
 #
-# @props: a dictionary of properties to be passed to the backend
+# @props: a dictionary of properties to be passed to the backend. Deprecated
+#         since 5.0, specify the properties on the top level instead. It is an
+#         error to specify the same option both on the top level and in @props.
+#
+# Additional arguments depend on qom-type and are passed to the backend
+# unchanged.
 #
 # Returns: Nothing on success
 #          Error if @qom-type is not a valid class name
@@ -221,12 +226,13 @@ 
 #
 # -> { "execute": "object-add",
 #      "arguments": { "qom-type": "rng-random", "id": "rng1",
-#                     "props": { "filename": "/dev/hwrng" } } }
+#                     "filename": "/dev/hwrng" } }
 # <- { "return": {} }
 #
 ##
 { 'command': 'object-add',
-  'data': {'qom-type': 'str', 'id': 'str', '*props': 'any'} }
+  'data': {'qom-type': 'str', 'id': 'str', '*props': 'any'},
+  'gen': false } # so we can get the additional arguments
 
 ##
 # @object-del:
diff --git a/docs/system/deprecated.rst b/docs/system/deprecated.rst
index 1eaa559079..6c1d9034d9 100644
--- a/docs/system/deprecated.rst
+++ b/docs/system/deprecated.rst
@@ -190,6 +190,11 @@  Use ``migrate-set-parameters`` instead.
 
 Use ``migrate-set-parameters`` and ``query-migrate-parameters`` instead.
 
+``object-add`` option ``props`` (since 5.0)
+'''''''''''''''''''''''''''''''''''''''''''
+
+Specify the properties for the object as top-level arguments instead.
+
 ``query-block`` result field ``dirty-bitmaps[i].status`` (since 4.0)
 ''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''
 
diff --git a/include/qom/object_interfaces.h b/include/qom/object_interfaces.h
index 3e4e1d928b..6f92f3cebb 100644
--- a/include/qom/object_interfaces.h
+++ b/include/qom/object_interfaces.h
@@ -162,4 +162,11 @@  void user_creatable_del(const char *id, Error **errp);
  */
 void user_creatable_cleanup(void);
 
+/**
+ * qmp_object_add:
+ *
+ * QMP command handler for object-add. See the QAPI schema for documentation.
+ */
+void qmp_object_add(QDict *qdict, QObject **ret_data, Error **errp);
+
 #endif
diff --git a/hw/block/xen-block.c b/hw/block/xen-block.c
index 686bbc3f0d..3885464513 100644
--- a/hw/block/xen-block.c
+++ b/hw/block/xen-block.c
@@ -18,6 +18,7 @@ 
 #include "qapi/visitor.h"
 #include "qapi/qmp/qdict.h"
 #include "qapi/qmp/qstring.h"
+#include "qom/object_interfaces.h"
 #include "hw/xen/xen_common.h"
 #include "hw/block/xen_blkif.h"
 #include "hw/qdev-properties.h"
@@ -858,10 +859,18 @@  static XenBlockIOThread *xen_block_iothread_create(const char *id,
 {
     XenBlockIOThread *iothread = g_new(XenBlockIOThread, 1);
     Error *local_err = NULL;
+    QDict *opts;
+    QObject *ret_data;
 
     iothread->id = g_strdup(id);
 
-    qmp_object_add(TYPE_IOTHREAD, id, false, NULL, &local_err);
+    opts = qdict_new();
+    qdict_put_str(opts, "qom-type", TYPE_IOTHREAD);
+    qdict_put_str(opts, "id", id);
+    qmp_object_add(opts, &ret_data, &local_err);
+    qobject_unref(opts);
+    qobject_unref(ret_data);
+
     if (local_err) {
         error_propagate(errp, local_err);
 
diff --git a/monitor/misc.c b/monitor/misc.c
index 6c41293102..1748ab3911 100644
--- a/monitor/misc.c
+++ b/monitor/misc.c
@@ -248,6 +248,8 @@  static void monitor_init_qmp_commands(void)
                          QCO_NO_OPTIONS);
     qmp_register_command(&qmp_commands, "netdev_add", qmp_netdev_add,
                          QCO_NO_OPTIONS);
+    qmp_register_command(&qmp_commands, "object-add", qmp_object_add,
+                         QCO_NO_OPTIONS);
 
     QTAILQ_INIT(&qmp_cap_negotiation_commands);
     qmp_register_command(&qmp_cap_negotiation_commands, "qmp_capabilities",
diff --git a/qom/qom-qmp-cmds.c b/qom/qom-qmp-cmds.c
index 6136efec16..49db926fcc 100644
--- a/qom/qom-qmp-cmds.c
+++ b/qom/qom-qmp-cmds.c
@@ -14,6 +14,7 @@ 
  */
 
 #include "qemu/osdep.h"
+#include "block/qdict.h"
 #include "hw/qdev-core.h"
 #include "qapi/error.h"
 #include "qapi/qapi-commands-qdev.h"
@@ -240,13 +241,34 @@  ObjectPropertyInfoList *qmp_qom_list_properties(const char *typename,
     return prop_list;
 }
 
-void qmp_object_add(const char *type, const char *id,
-                    bool has_props, QObject *props, Error **errp)
+void qmp_object_add(QDict *qdict, QObject **ret_data, Error **errp)
 {
+    QObject *props;
     QDict *pdict;
     Visitor *v;
     Object *obj;
+    const char *type;
+    const char *id;
 
+    type = qdict_get_try_str(qdict, "qom-type");
+    if (!type) {
+        error_setg(errp, QERR_MISSING_PARAMETER, "qom-type");
+        return;
+    } else {
+        type = g_strdup(type);
+        qdict_del(qdict, "qom-type");
+    }
+
+    id = qdict_get_try_str(qdict, "id");
+    if (!id) {
+        error_setg(errp, QERR_MISSING_PARAMETER, "id");
+        return;
+    } else {
+        id = g_strdup(id);
+        qdict_del(qdict, "id");
+    }
+
+    props = qdict_get(qdict, "props");
     if (props) {
         pdict = qobject_to(QDict, props);
         if (!pdict) {
@@ -254,17 +276,23 @@  void qmp_object_add(const char *type, const char *id,
             return;
         }
         qobject_ref(pdict);
-    } else {
-        pdict = qdict_new();
+        qdict_del(qdict, "props");
+        qdict_join(qdict, pdict, false);
+        if (qdict_size(pdict) != 0) {
+            error_setg(errp, "Option in 'props' conflicts with top level");
+            qobject_unref(pdict);
+            return;
+        }
+        qobject_unref(pdict);
     }
 
-    v = qobject_input_visitor_new(QOBJECT(pdict));
-    obj = user_creatable_add_type(type, id, pdict, v, errp);
+    v = qobject_input_visitor_new(QOBJECT(qdict));
+    obj = user_creatable_add_type(type, id, qdict, v, errp);
     visit_free(v);
     if (obj) {
         object_unref(obj);
     }
-    qobject_unref(pdict);
+    *ret_data = QOBJECT(qdict_new());
 }
 
 void qmp_object_del(const char *id, Error **errp)