Re: [Qemu-devel] [PATCH v2 0/5] Trivial patch about qemu-char

[zhanghailiang <zhang.zhanghailiang@huawei.com>]

On 2014/11/3 23:33, Markus Armbruster wrote:
> zhanghailiang <zhang.zhanghailiang@huawei.com> writes:
>
>> On 2014/11/3 18:03, Michael Tokarev wrote:
>>> 03.11.2014 12:44, zhanghailiang wrote:
>>>> Patch 1~3 fix wrong check about in-parameter.
>>>> The last two patches convert some open functions to use Error API.
>>>>
>>>> v2:
>>>> - don't use error_setg when followed by exit(), it does not report
>>>> an error (Eric Blake)
>>>> - check the parameter in qemu_chr_parse_* functions and remove the
>>>> check in qemu_chr_open_* functions. (Michael Tokarev)
>>>>
>>>> Thanks very much for their reviews and suggestions;)
>>>
>>> Thank you for doing all this.  I think I can apply this but with folding
>>> patches 1 and 2 into one, -- it is better to see them both in the same
>>> context, just like you did in patch 3.  If that's okay with you, I'll
>>> just apply it like this.
>>>
>>
>> Sure, I'm Ok with it;)
>>
>>> Speaking of Error API -- what is the general consensus of this?  We have
>>> TONS of various way to report errors now, and we had several incarnations
>>> of various error APIs too, are we going to use some common API finally,
>>> or is the conversion endless, expecting new APIs to arrive?
>
> I hope we're done screwing up error APIs, and now we "only" have to
> convert code from the APIs we don't want used to the ones we do want
> used.  Quote http://wiki.qemu.org/CodeTransitions#Error_reporting:
>
> * Avoid ErrorClass values other than ERROR_CLASS_GENERIC_ERROR unless
>    you have a specific reason.  Prefer error_setg() & friends over
>    error_set() & friends.
>
> * The QError macros QERR_ are a hack to help with the transition to the
>    current error.h API (human-readable message rather than JSON argument,
>    see commit df1e608).  Avoid them in new code, use simple message
>    strings instead.
>
> * qerror_report() is a transitional interface to help with converting
>    existing HMP commands to QMP.  It should not be used elsewhere.  Use
>    Error objects instead with error_propagate() and error_setg() instead.
>
> * Use error_report() & friends instead of fprintf(stderr, ...).  Unlike
>    fprintf(), it does the right thing in human monitor context.  It also
>    adds program name and error location when appropriate, and supports
>    timestamps (-msg timestamp=on).
>
> End quote.  More questions?
>
>> Yes, this confused me, besides, error_setg will add a '\n' in the end, but
>> for example, print_allowed_subtypes where use fprintf(stderr,..)
>> without '\n' in the middle places,
>> IMHO, maybe it makes sense to return this to the higher caller.
>>
>>> And one more thing.  Patch 4 does this:
>>>
>>> @@ -1388,8 +1388,8 @@ static CharDriverState
>>> *qemu_chr_open_pty(const char *id,
>>>        ret->pty = g_strdup(pty_name);
>>>        ret->has_pty = true;
>>>
>>> -    fprintf(stderr, "char device redirected to %s (label %s)\n",
>>> -            pty_name, id);
>>> +    error_report("char device redirected to %s (label %s)",
>>> +                 pty_name, id);
>>>
>>>        s = g_malloc0(sizeof(PtyCharDriver));
>>>        chr->opaque = s;
>>>
>>> This is not really correct.  First it is not really an error, it is
>>> an informational message.  But also, there are many scripts out there
>>> who expects this very format of this message to find the entry to
>>> that char device.  Converting this message to error_report() changes
>>> its format, so scrips will be unable to find the device anymore.
>>>
>>> Take a look at history of this place, -- I remember there was a rather
>>> hot discussion when the last part, "(label %)", has been added (initial
>>> message was without this label).  Initial suggestion was to change
>>> wordin to 'char device $LABEL redirected to $DEVICE', but even if it
>>> is much more readable and correct, we agreed to add that "label" part
>>> to the end - not that it preserves original message, but at least it
>>> makes less scripts to fail...
>>>
>>
>> I got it, thanks very much for your explanation, and your patience;)
>
> Please use error_printf(), so it prints to the monitor when running on

Ah, i didn't notice this function before, great;) thanks.

> behalf of monitor command chardev-add.  Separate patch, because it's a
> separate bug fix.
>

OK, will do in V3.

>>> So at least this hunk should not be applied.  I think this place
>>> deserves a comment.
>>>
>>> I'm sorry for being so picky, but I think I give enough reasons
>>> explaining why, and these reasons are serious enough.
>>
>> It's okay, So what's your opinion about this series?
>> Should i merge patch 1 and 2 into one patch in V3?
>> Keep the other modifies except the incorrect modify in
>> qemu_chr_open_pty for patch 4?
>> If yes, i will send V3;)
>
> Sounds like a plan to me.
> .
>

Thanks,
zhanghailiang