On 09/24/2015 01:57 PM, Yuanhan Liu wrote:
> On Thu, Sep 24, 2015 at 01:34:31PM +0800, Jason Wang wrote:
>>
>> Some nitpicks and comments. If you plan to send another version, please
>> consider to fix them.
> I will, but I'd like to hold a while before getting more comments from
> Michael; I don't want to repost a whole new version too often for minor
> changes.
>
> BTW, Michael, is this patchset being ready for merge? If not, please
> kindly tell me what is wrong so that I can fix them. TBH, I'd really
> like to see it be merged soon, as I'm gonna have holidays soon for
> two weeks start from this Saturday. I could still reply emails, even
> make patches during that, but I guess I only have limited time for that.
>
>> Reviewed-by: Jason Wang <jasow...@redhat.com>
> Thank you!
>
>> Btw, it's better to extend current vhost-user unit test to support
>> multiqueue. Please consider this on top this series.
> Yeah, I will. But, may I do that later? (And if possible, after the
> merge?)
>
Yes, of course :)
>>> diff --git a/docs/specs/vhost-user.txt b/docs/specs/vhost-user.txt
>>> index 43db9b4..cfc9d41 100644
>>> --- a/docs/specs/vhost-user.txt
>>> +++ b/docs/specs/vhost-user.txt
>>> @@ -135,6 +135,21 @@ As older slaves don't support negotiating protocol
>>> features,
>>> a feature bit was dedicated for this purpose:
>>> #define VHOST_USER_F_PROTOCOL_FEATURES 30
>>>
[...]
>>>
>>> static void vhost_user_cleanup(NetClientState *nc)
>>> {
>>> VhostUserState *s = DO_UPCAST(VhostUserState, nc, nc);
>>>
>>> - vhost_user_stop(s);
>>> + if (s->vhost_net) {
>>> + vhost_net_cleanup(s->vhost_net);
>>> + s->vhost_net = NULL;
>>> + }
>>> +
>>> qemu_purge_queued_packets(nc);
>>> }
>>>
>>> @@ -95,59 +136,61 @@ static NetClientInfo net_vhost_user_info = {
>>> .has_ufo = vhost_user_has_ufo,
>>> };
>>>
>>> -static void net_vhost_link_down(VhostUserState *s, bool link_down)
>>> -{
>>> - s->nc.link_down = link_down;
>>> -
>>> - if (s->nc.peer) {
>>> - s->nc.peer->link_down = link_down;
>>> - }
>>> -
>>> - if (s->nc.info->link_status_changed) {
>>> - s->nc.info->link_status_changed(&s->nc);
>>> - }
>>> -
>>> - if (s->nc.peer && s->nc.peer->info->link_status_changed) {
>>> - s->nc.peer->info->link_status_changed(s->nc.peer);
>>> - }
>>> -}
>>> -
>>> static void net_vhost_user_event(void *opaque, int event)
>>> {
>>> - VhostUserState *s = opaque;
>>> + const char *name = opaque;
>>> + NetClientState *ncs[MAX_QUEUE_NUM];
>>> + VhostUserState *s;
>>> + Error *err = NULL;
>>> + int queues;
>>>
>>> + queues = qemu_find_net_clients_except(name, ncs,
>>> + NET_CLIENT_OPTIONS_KIND_NIC,
>>> + MAX_QUEUE_NUM);
>>> + s = DO_UPCAST(VhostUserState, nc, ncs[0]);
>>> switch (event) {
>>> case CHR_EVENT_OPENED:
>>> - vhost_user_start(s);
>>> - net_vhost_link_down(s, false);
>>> + if (vhost_user_start(queues, ncs) < 0) {
>>> + exit(1);
>> How about report a specific error instead of just abort here?
> There is an error report at vhost_user_start():
>
> error_report("you are asking more queues than "
> "supported: %d\n", max_queues);
>
> Or, do you mean something else?
>
>
> --yliu
Not sure, but we have error_abort and error_fatal. Markus may know more
about this.
[...]
