qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH 09/19] sockets: Chardev open error reporting, so


From: Markus Armbruster
Subject: Re: [Qemu-devel] [PATCH 09/19] sockets: Chardev open error reporting, sockets part
Date: Wed, 15 Feb 2012 14:33:22 +0100
User-agent: Gnus/5.13 (Gnus v5.13) Emacs/23.3 (gnu/linux)

Anthony Liguori <address@hidden> writes:

> On 02/14/2012 11:24 AM, Markus Armbruster wrote:
>> Markus Armbruster<address@hidden>  writes:
>>
>>> Anthony Liguori<address@hidden>  writes:
>> [Anthony asking for error_set() instead of error_report()...]
>>>> Basically, same thing here and the remaining functions.  Let's not
>>>> introduce additional uses of error_report().
>>>>
>>>> That said, I imagine you don't want to introduce a bunch of error
>>>> types for these different things and that's probably not productive
>>>> anyway.
>> [...]
>>>> So let's compromise and introduce a generic QERR_INTERNAL_ERROR that
>>>> takes a single human readable string as an argument.  We can have a
>>>> wrapper for it that also records location information in the error
>>>> object.
>>>
>>> This series goes from stderr to error_report().  That's a relatively
>>> simple step, which makes it relatively easy to review.  I'm afraid
>>> moving all the way to error.h in one step wouldn't be as easy.  Kevin
>>> suggests to do it in a follow-up series, and I agree.
>
> The trouble I have is not about doing things incrementally, but rather
> touching a lot of code incrementally.  Most of the code you touch
> could be done incrementally with error_set().
>
> For instance, you could touch inet_listen_opts() and just add an Error
> ** as the last argument.  You can change all callers to simply do:
>
> Error *err = NULL;
>
> ...
> inet_listen_opts(..., &err);
> if (err) {
>    error_report_err(err);
>    return -1;
> }
>
> And it's not really all that different from the series as it stands
> today.  I agree that aggressively refactoring error propagation is
> probably not necessary as a first step, but if we're going to touch a
> lot of code, we should do it in a way that we don't have to
> immediately touch it again next.

Well, the series adds 47 calls of error_report() to five files out of
1850.

>>> Can you point to an existing conversion from error_report() to error.h,
>>> to give us an idea how it's supposed to be done?
>>
>> Ping?
>
> Sorry, I mentally responded bug neglected to actually respond.
>
> All of the QMP work that Luiz is doing effectively does this so there
> are ample examples right now.  The change command is probably a good
> place to start.

Thanks.

Unfortunately, I'm out of time on this one, so if you're unwilling to
accept this admittedly incremental improvement without substantial
rework, I'll have to let it rot in peace.

We might want a QMP commands to add character devices some day.  Perhaps
the person doing it will still be able to find these patches, and get
some use out of them.

Patches 01-08,14 don't add error_report() calls.  What about committing
them?  The commit messages would need to be reworded not to promise
goodies from the other patches, though.



reply via email to

[Prev in Thread] Current Thread [Next in Thread]