[Top][All Lists]

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

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

From: zhanghailiang
Subject: Re: [Qemu-trivial] [Qemu-devel] [PATCH v2 0/5] Trivial patch about qemu-char
Date: Tue, 4 Nov 2014 10:20:57 +0800
User-agent: Mozilla/5.0 (Windows NT 6.1; rv:31.0) Gecko/20100101 Thunderbird/31.1.1

On 2014/11/3 23:33, Markus Armbruster wrote:
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.

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


reply via email to

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