Re: [PATCH v9 07/20] multi-process: define transmission functions in remote

[Elena Ufimtseva]

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