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

[zhanghailiang]

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