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

[zhanghailiang]

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?

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;)

> 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;)

Thanks,
zhanghailiang