Re: [RFC] vhost user: add error handling for fd > 1023

[Patrik Andersson R <patrik.r.andersson@ericsson.com>]

Yes, that is correct. Closing the socket on failure needs to be added.

Regards,

Patrik



On 04/11/2016 11:34 AM, Christian Ehrhardt wrote:
> I like the approach as well to go for the fix for robustness first.
>
> I was accidentally able to find another testcase to hit the same root 
> cause.
> Adding guests with 15 vhost_user based NICs each while having rxq for 
> openvswitch-dpdk set to 4 and multiqueue for the guest devices at 4 
> already breaks when adding the thirds such guests.
> That is way earlier than I would have expected the fd's to be 
> exhausted but still the same root cause, so just another test for the 
> same.
>
> In prep to the wider check to the patch a minor review question from me:
> On the section of rte_vhost_driver_register that now detects if there 
> were issues we might want to close the socketfd as well when bailing out.
> Otherwise we would just have another source of fd leaks or would that 
> be reused later on even now that we have freed vserver-path and 
> vserver itself?
>
>        ret = fdset_add(&g_vhost_server.fdset, vserver->listenfd,
>  vserver_new_vq_conn, NULL, vserver);
>        if (ret < 0) {
>  pthread_mutex_unlock(&g_vhost_server.server_mutex);
>  RTE_LOG(ERR, VHOST_CONFIG,
>        "failed to add listen fd %d to vhost server fdset\n",
>        vserver->listenfd);
>  free(vserver->path);
> +  close(vserver->listenfd);
>  free(vserver);
>                return -1;
>        }
>
>
> Christian Ehrhardt
> Software Engineer, Ubuntu Server
> Canonical Ltd
>
> On Mon, Apr 11, 2016 at 8:06 AM, Patrik Andersson R 
> <patrik.r.andersson@ericsson.com 
> <mailto:patrik.r.andersson@ericsson.com>> wrote:
>
>     I fully agree with this course of action.
>
>     Thank you,
>
>     Patrik
>
>
>
>     On 04/08/2016 08:47 AM, Xie, Huawei wrote:
>
>         On 4/7/2016 10:52 PM, Christian Ehrhardt wrote:
>
>             I totally agree to that there is no deterministic rule
>             what to expect.
>             The only rule is that #fd certainly always is >
>             #vhost_user devices.
>             In various setup variants I've crossed fd 1024 anywhere
>             between 475
>             and 970 vhost_user ports.
>
>             Once the discussion continues and we have an updates
>             version of the
>             patch with some more agreement I hope I can help to test it.
>
>         Thanks. Let us first temporarily fix this problem for
>         robustness, then
>         we consider whether upgrade to (e)poll.
>         Will check the patch in detail later. Basically it should work
>         but need
>         check whether we need extra fixes elsewhere.
>
>
>