[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v9 07/20] multi-process: define transmission functions in rem
From: |
Elena Ufimtseva |
Subject: |
Re: [PATCH v9 07/20] multi-process: define transmission functions in remote |
Date: |
Thu, 24 Sep 2020 10:18:49 -0700 |
User-agent: |
Mutt/1.10.1 (2018-07-13) |
On Wed, Sep 23, 2020 at 03:02:46PM +0100, Stefan Hajnoczi wrote:
> On Thu, Aug 27, 2020 at 11:12:18AM -0700, elena.ufimtseva@oracle.com wrote:
> > TODO: Avoid the aio_poll by entering the co-routines
> > from the higher level to avoid aio_poll.
>
> The monitor is unresponsive during the aio_poll() loop. Is this a
> blocker for you?
>
Hi Stefan
No, not a blocker, had to leave out removal of aio_poll for the next round.
> Running all mpqemu communication in a coroutine as mentioned in this
> TODO is a cleaner solution. Then this patch will be unnecessary.
>
Yes, thank you, it will go away in v10.
> > +static void coroutine_fn mpqemu_msg_send_co(void *data)
> > +{
> > + MPQemuRequest *req = (MPQemuRequest *)data;
> > + Error *local_err = NULL;
> > +
> > + mpqemu_msg_send(req->msg, req->ioc, &local_err);
> > + if (local_err) {
> > + error_report("ERROR: failed to send command to remote %d, ",
> > + req->msg->cmd);
> > + req->finished = true;
> > + req->error = -EINVAL;
> > + return;
>
> local_err is leaked.
>
> > + }
> > +
> > + req->finished = true;
> > +}
> > +
> > +void mpqemu_msg_send_in_co(MPQemuRequest *req, QIOChannel *ioc,
> > + Error **errp)
> > +{
> > + Coroutine *co;
> > +
> > + if (!req->ioc) {
> > + if (errp) {
> > + error_setg(errp, "Channel is set to NULL");
> > + } else {
> > + error_report("Channel is set to NULL");
> > + }
>
> The caller should provide an errp if they are interested in the error
> message. Duplicating error messages is messy.
>
> > +static void coroutine_fn mpqemu_msg_recv_co(void *data)
> > +{
> > + MPQemuRequest *req = (MPQemuRequest *)data;
> > + Error *local_err = NULL;
> > +
> > + mpqemu_msg_recv(req->msg, req->ioc, &local_err);
> > + if (local_err) {
> > + error_report("ERROR: failed to send command to remote %d, ",
> > + req->msg->cmd);
> > + req->finished = true;
> > + req->error = -EINVAL;
> > + return;
>
> local_err is leaked.
Thank you for reviewing these, will fix If the code above will be re-used.
Elena