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.
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?
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...
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.
Thanks,
/mjt
