Re: [Qemu-devel] [PATCH v10 6/7] vhost-user: add multiple queue support

[Yuanhan Liu]

On Tue, Sep 22, 2015 at 06:14:14PM +0800, Jason Wang wrote:
> 
> 
[...]
> > -static void net_vhost_link_down(VhostUserState *s, bool link_down)
> > +static void net_vhost_link_down(int queues, NetClientState *ncs[],
> > +                                bool link_down)
> >  {
> > -    s->nc.link_down = link_down;
> > +    NetClientState *nc;
> > +    int i;
> >  
> > -    if (s->nc.peer) {
> > -        s->nc.peer->link_down = link_down;
> > -    }
> > +    for (i = 0; i < queues; i++) {
> > +        nc = ncs[i];
> >  
> > -    if (s->nc.info->link_status_changed) {
> > -        s->nc.info->link_status_changed(&s->nc);
> > -    }
> > +        nc->link_down = link_down;
> > +
> > +        if (nc->peer) {
> > +            nc->peer->link_down = link_down;
> > +        }
> > +
> > +        if (nc->info->link_status_changed) {
> > +            nc->info->link_status_changed(nc);
> > +        }
> >  
> > -    if (s->nc.peer && s->nc.peer->info->link_status_changed) {
> > -        s->nc.peer->info->link_status_changed(s->nc.peer);
> > +        if (nc->peer && nc->peer->info->link_status_changed) {
> > +            nc->peer->info->link_status_changed(nc->peer);
> > +        }
> >      }
> >  }
> 
> The semantics is a little bit difference with qmp_set_link. What's the
> reason for this? If there's no specific reason, probably, we could just
> reuse qmp_set_link() (or part of) here?

I did this just because I refactored the vhost_user init code, making
the char dev handler to initiate all ncs. Hence I turned net_vhost_link_down()
to handle all ncs, just like what I did for vhost_user_start().

And TBH, I'm not aware of qmp_set_link() before, and after reading the
code, I do think we can simply invoke it instead. And thanks for the
info.

> 
> Other looks good to me.

Thanks for the review. May I add your reviewed-by if I fix above issue?

        --yliu