Add a pair to &error_warn helper, to reduce the pattern like

    Error *local_err = NULL;

    ...

    if (!foo(..., &local_err)) {
        error_report_err(local_err);
        return false;
    }

to simply

    if (!foo(..., &error_reporter)) {
        return false;
    }

Of course, for new interfaces, it's better to always support and handle
errp argument. But when have to rework the old ones, it's not always
feasible to convert everything to support/handle errp.

The new helper is used in following commits.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
---
 include/qapi/error.h | 6 ++++++
 util/error.c         | 3 +++
 2 files changed, 9 insertions(+)

diff --git a/include/qapi/error.h b/include/qapi/error.h
index 41e3816380..cea95f5c36 100644
--- a/include/qapi/error.h
+++ b/include/qapi/error.h
@@ -539,6 +539,12 @@ G_DEFINE_AUTO_CLEANUP_CLEAR_FUNC(ErrorPropagator, error_propagator_cleanup);
  */
 extern Error *error_warn;
 
+/*
+ * Special error destination to report on error.
+ * See error_setg() and error_propagate() for details.
+ */
+extern Error *error_reporter;
+
 /*
  * Special error destination to abort on error.
  * See error_setg() and error_propagate() for details.
diff --git a/util/error.c b/util/error.c
index daea2142f3..5160435c86 100644
--- a/util/error.c
+++ b/util/error.c
@@ -20,6 +20,7 @@
 Error *error_abort;
 Error *error_fatal;
 Error *error_warn;
+Error *error_reporter;
 
 static void error_handle(Error **errp, Error *err)
 {
@@ -43,6 +44,8 @@ static void error_handle(Error **errp, Error *err)
     }
     if (errp == &error_warn) {
         warn_report_err(err);
+    } else if (errp == &error_reporter) {
+        error_report_err(err);
     } else if (errp && !*errp) {
         *errp = err;
     } else {
-- 
2.48.1

On Mon, Sep 15, 2025 at 04:22:01PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> Add a pair to &error_warn helper, to reduce the pattern like
> 
>     Error *local_err = NULL;
> 
>     ...
> 
>     if (!foo(..., &local_err)) {
>         error_report_err(local_err);
>         return false;
>     }
> 
> to simply
> 
>     if (!foo(..., &error_reporter)) {
>         return false;
>     }
> 
> Of course, for new interfaces, it's better to always support and handle
> errp argument. But when have to rework the old ones, it's not always
> feasible to convert everything to support/handle errp.
> 
> The new helper is used in following commits.

Could we add some explanation of why we need this if we already have
error_warn?

I don't see much difference except error_warn will prepend it with
"warning: ", but that doesn't sound like a real problem..

> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> ---
>  include/qapi/error.h | 6 ++++++
>  util/error.c         | 3 +++
>  2 files changed, 9 insertions(+)
> 
> diff --git a/include/qapi/error.h b/include/qapi/error.h
> index 41e3816380..cea95f5c36 100644
> --- a/include/qapi/error.h
> +++ b/include/qapi/error.h
> @@ -539,6 +539,12 @@ G_DEFINE_AUTO_CLEANUP_CLEAR_FUNC(ErrorPropagator, error_propagator_cleanup);
>   */
>  extern Error *error_warn;
>  
> +/*
> + * Special error destination to report on error.
> + * See error_setg() and error_propagate() for details.
> + */
> +extern Error *error_reporter;
> +
>  /*
>   * Special error destination to abort on error.
>   * See error_setg() and error_propagate() for details.
> diff --git a/util/error.c b/util/error.c
> index daea2142f3..5160435c86 100644
> --- a/util/error.c
> +++ b/util/error.c
> @@ -20,6 +20,7 @@
>  Error *error_abort;
>  Error *error_fatal;
>  Error *error_warn;
> +Error *error_reporter;
>  
>  static void error_handle(Error **errp, Error *err)
>  {
> @@ -43,6 +44,8 @@ static void error_handle(Error **errp, Error *err)
>      }
>      if (errp == &error_warn) {
>          warn_report_err(err);
> +    } else if (errp == &error_reporter) {
> +        error_report_err(err);
>      } else if (errp && !*errp) {
>          *errp = err;
>      } else {
> -- 
> 2.48.1
> 

-- 
Peter Xu

On 15.09.25 18:45, Peter Xu wrote:
> On Mon, Sep 15, 2025 at 04:22:01PM +0300, Vladimir Sementsov-Ogievskiy wrote:
>> Add a pair to &error_warn helper, to reduce the pattern like
>>
>>      Error *local_err = NULL;
>>
>>      ...
>>
>>      if (!foo(..., &local_err)) {
>>          error_report_err(local_err);
>>          return false;
>>      }
>>
>> to simply
>>
>>      if (!foo(..., &error_reporter)) {
>>          return false;
>>      }
>>
>> Of course, for new interfaces, it's better to always support and handle
>> errp argument. But when have to rework the old ones, it's not always
>> feasible to convert everything to support/handle errp.
>>
>> The new helper is used in following commits.
> Could we add some explanation of why we need this if we already have
> error_warn?
> 
> I don't see much difference except error_warn will prepend it with
> "warning: ", but that doesn't sound like a real problem..

Yes, seems it's the only difference.

For me it seems strange to call it "warning", when we actually go to failure branch of the code logic.
Finally, we do have error_report() and warn_report(). Seems not a problem to have corresponding "magic" variables.

If have only one special error variable to simply report the error, I'd prefer it not have "warning: " prefix.
I.e. drop error_warn, and keep only error_reporter (or some better name?).



-- 
Best regards,
Vladimir

On Mon, Sep 15, 2025 at 09:14:18PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> On 15.09.25 18:45, Peter Xu wrote:
> > On Mon, Sep 15, 2025 at 04:22:01PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> > > Add a pair to &error_warn helper, to reduce the pattern like
> > > 
> > >      Error *local_err = NULL;
> > > 
> > >      ...
> > > 
> > >      if (!foo(..., &local_err)) {
> > >          error_report_err(local_err);
> > >          return false;
> > >      }
> > > 
> > > to simply
> > > 
> > >      if (!foo(..., &error_reporter)) {
> > >          return false;
> > >      }
> > > 
> > > Of course, for new interfaces, it's better to always support and handle
> > > errp argument. But when have to rework the old ones, it's not always
> > > feasible to convert everything to support/handle errp.
> > > 
> > > The new helper is used in following commits.
> > Could we add some explanation of why we need this if we already have
> > error_warn?
> > 
> > I don't see much difference except error_warn will prepend it with
> > "warning: ", but that doesn't sound like a real problem..
> 
> Yes, seems it's the only difference.
> 
> For me it seems strange to call it "warning", when we actually go to failure branch of the code logic.
> Finally, we do have error_report() and warn_report(). Seems not a problem to have corresponding "magic" variables.
> 
> If have only one special error variable to simply report the error, I'd prefer it not have "warning: " prefix.
> I.e. drop error_warn, and keep only error_reporter (or some better name?).

FWIW, this whole debate is liable to be a bit of a can of worms that
will delay your series from getting merged.

Can I suggest you repost this without &error_reporter usage, and then
have a separate standalone series that proposes error_report, and
converts a handful of files to demonstrate its usage.

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Daniel P. Berrangé <berrange@redhat.com> writes:

> On Mon, Sep 15, 2025 at 09:14:18PM +0300, Vladimir Sementsov-Ogievskiy wrote:
>> On 15.09.25 18:45, Peter Xu wrote:
>> > On Mon, Sep 15, 2025 at 04:22:01PM +0300, Vladimir Sementsov-Ogievskiy wrote:
>> > > Add a pair to &error_warn helper, to reduce the pattern like
>> > > 
>> > >      Error *local_err = NULL;
>> > > 
>> > >      ...
>> > > 
>> > >      if (!foo(..., &local_err)) {
>> > >          error_report_err(local_err);
>> > >          return false;
>> > >      }
>> > > 
>> > > to simply
>> > > 
>> > >      if (!foo(..., &error_reporter)) {
>> > >          return false;
>> > >      }
>> > > 
>> > > Of course, for new interfaces, it's better to always support and handle
>> > > errp argument. But when have to rework the old ones, it's not always
>> > > feasible to convert everything to support/handle errp.
>> > > 
>> > > The new helper is used in following commits.
>> > 
>> > Could we add some explanation of why we need this if we already have
>> > error_warn?
>> > 
>> > I don't see much difference except error_warn will prepend it with
>> > "warning: ", but that doesn't sound like a real problem..
>> 
>> Yes, seems it's the only difference.
>> 
>> For me it seems strange to call it "warning", when we actually go to failure branch of the code logic.
>> Finally, we do have error_report() and warn_report(). Seems not a problem to have corresponding "magic" variables.
>> 
>> If have only one special error variable to simply report the error, I'd prefer it not have "warning: " prefix.
>> I.e. drop error_warn, and keep only error_reporter (or some better name?).
>
> FWIW, this whole debate is liable to be a bit of a can of worms that
> will delay your series from getting merged.
>
> Can I suggest you repost this without &error_reporter usage, and then
> have a separate standalone series that proposes error_report, and
> converts a handful of files to demonstrate its usage.

Seconded.

Please note the killing of &error_warn is in progress:

    Subject: [PATCH v2 00/12] Error reporting cleanup, a fix, and &error_warn removal
    Date: Wed, 17 Sep 2025 13:51:55 +0200
    Message-ID: <20250917115207.1730186-1-armbru@redhat.com>
    https://lore.kernel.org/qemu-devel/20250917115207.1730186-1-armbru@redhat.com/

Rationale in PATCH 12.

I doubt additional special error destinations are a good idea.
&error_abort definitely is a good idea.  &error_fatal is problematic,
but widely used.

Akihiko Odaki <odaki@rsg.ci.i.u-tokyo.ac.jp> suggested to add one that
at a glance looks just like yours.  Please see my reply at

    Message-ID: <87a548sgjl.fsf@pond.sub.org>
    https://lore.kernel.org/qemu-devel/87a548sgjl.fsf@pond.sub.org/

On 15.09.25 21:29, Daniel P. Berrangé wrote:
> On Mon, Sep 15, 2025 at 09:14:18PM +0300, Vladimir Sementsov-Ogievskiy wrote:
>> On 15.09.25 18:45, Peter Xu wrote:
>>> On Mon, Sep 15, 2025 at 04:22:01PM +0300, Vladimir Sementsov-Ogievskiy wrote:
>>>> Add a pair to &error_warn helper, to reduce the pattern like
>>>>
>>>>       Error *local_err = NULL;
>>>>
>>>>       ...
>>>>
>>>>       if (!foo(..., &local_err)) {
>>>>           error_report_err(local_err);
>>>>           return false;
>>>>       }
>>>>
>>>> to simply
>>>>
>>>>       if (!foo(..., &error_reporter)) {
>>>>           return false;
>>>>       }
>>>>
>>>> Of course, for new interfaces, it's better to always support and handle
>>>> errp argument. But when have to rework the old ones, it's not always
>>>> feasible to convert everything to support/handle errp.
>>>>
>>>> The new helper is used in following commits.
>>> Could we add some explanation of why we need this if we already have
>>> error_warn?
>>>
>>> I don't see much difference except error_warn will prepend it with
>>> "warning: ", but that doesn't sound like a real problem..
>>
>> Yes, seems it's the only difference.
>>
>> For me it seems strange to call it "warning", when we actually go to failure branch of the code logic.
>> Finally, we do have error_report() and warn_report(). Seems not a problem to have corresponding "magic" variables.
>>
>> If have only one special error variable to simply report the error, I'd prefer it not have "warning: " prefix.
>> I.e. drop error_warn, and keep only error_reporter (or some better name?).
> 
> FWIW, this whole debate is liable to be a bit of a can of worms that
> will delay your series from getting merged.

Honest

> 
> Can I suggest you repost this without &error_reporter usage, and then
> have a separate standalone series that proposes error_report, and
> converts a handful of files to demonstrate its usage.
> 

Agree.


-- 
Best regards,
Vladimir