Date: Mon, 26 Mar 2018 17:13:51 +0800
From: Peter Xu <peterx@redhat.com>
Message-ID: <20180326091351.GD17789@xz-mi>
References: <20180326063901.27425-1-peterx@redhat.com>
	<20180326063901.27425-4-peterx@redhat.com>
	<CAMxuvawX2yzMpMYNsB6b-8NP6iXm-ugjE5TUgj5KkgiEEUApFA@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <CAMxuvawX2yzMpMYNsB6b-8NP6iXm-ugjE5TUgj5KkgiEEUApFA@mail.gmail.com>
Content-Transfer-Encoding: quoted-printable
Subject: Re: [Qemu-devel] [PATCH for-2.12 3/8] monitor: new parameter "x-oob"
To: =?utf-8?Q?Marc-Andr=C3=A9?= Lureau <marcandre.lureau@redhat.com>
Cc: qemu-devel <qemu-devel@nongnu.org>, Eric Blake <eblake@redhat.com>, Markus Armbruster <armbru@redhat.com>, Stefan Hajnoczi <stefanha@redhat.com>, "Dr . David Alan Gilbert" <dgilbert@redhat.com>

On Mon, Mar 26, 2018 at 11:10:30AM +0200, Marc-Andr=C3=A9 Lureau wrote:

[...]

> > @@ -4568,12 +4569,26 @@ static void monitor_qmp_setup_handlers_bh(voi=
d *opaque)
> >  void monitor_init(Chardev *chr, int flags)
> >  {
> >      Monitor *mon =3D g_malloc(sizeof(*mon));
> > +    bool use_readline =3D flags & MONITOR_USE_READLINE;
> > +    bool use_oob =3D flags & MONITOR_USE_OOB;
> > +
> > +    if (use_oob) {
> > +        if (CHARDEV_IS_MUX(chr)) {
> > +            error_report("Monitor Out-Of-Band is not supported with =
"
> > +                         "MUX typed chardev backend");
> > +            exit(1);
> > +        }
> > +        if (use_readline) {
> > +            error_report("Monitor Out-Of-band is only supported by Q=
MP");
> > +            exit(1);
> > +        }
> > +    }
>=20
> I would rather see the error reporting / exit in vl.c:mon_init_func()
> function, to have a single place for exit()

But there can be other callers for monitor_init() so I would assume
checking it here would be safer (though for now indeed mon_init_func()
is the only one that can pass OOB flag in).

[...]

> Other than that, it looks fine.
> Reviewed-by: Marc-Andr=C3=A9 Lureau <marcandre.lureau@redhat.com>

Thanks for your quick reviews!

--=20
Peter Xu