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

[Markus Armbruster]

zhanghailiang <address@hidden> 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
behalf of monitor command chardev-add.  Separate patch, because it's a
separate bug fix.

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