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 :|
