Hi Ansis,
Ansis Atteka <ansisatt...@gmail.com> writes:
> On 2 February 2016 at 17:56, Daniele Di Proietto <diproiet...@vmware.com>
> wrote:
>
>> If ovs-vswitchd crashes, it will not be able to recreate the same
>> vhost user ports, since the socket will still be in the file system.
>>
>> This commit introduces an unlink() before creation to remove an eventual
>> preexisting vhost user socket.
>>
>> This mimics the behavior of the OpenFlow management socket for a bridge.
>>
>> Signed-off-by: Daniele Di Proietto <diproiet...@vmware.com>
>> ---
>> There have been security concerns about the introduction of a similar
>> mechanism, because the port name is basically untrusted input.
>>
>> I decided to post this anyway since:
>> * The previous commit introduces a check to discard port names with '/'
>> * This is how the OpenFlow management socket is creation is handled.
>> As pointed out by Ben, though, an OpenFlow socket always ends in '.mgmt'
>> while a vhost user socket doesn't have any suffix.
>>
>
> How about introducing something like:
> unlink_only_if_is_orphaned_socket(...)?
>
> ls -la is able to indicate that inode is socket (the s prefix):
> aatteka@aatteka-PowerEdge-T110:~/Git/openvswitch$ ls -la
> /var/run/openvswitch/
> srwxr-x--- 1 root root 0 Feb 3 14:43 db.sock
>
> And ss -p is able to confirm existence of the other end of the socket:
> aatteka@aatteka-PowerEdge-T110:~/Git/openvswitch$ sudo ss -p | grep
> "db.sock"
> u_str ESTAB 0 0 /var/run/openvswitch/db.sock 38934130
> * 38933374 users:(("ovsdb-server",pid=3658,fd=18))
>
> I did not look into "ss -p" internals so I can't comment about portability
> or complexity if you want to pursue this path. Also, I believe it still
> does not solve a minor leak problem if ovs-vswitchd was abruptly killed and
> the bridge did not show up in OVSDB on the next ovs-vswitchd startup hence
> relics of old socket remained there. I don't have a clean solution for this
> unless there is convention that OVS can wipe out all contents of a
> dedicated directory where Unix Domain Sockets are created.
IMHO, this is all just guessing games - testing this way is non-atomic. There
must be a way to have DPDK (or even if there is some non-dpdk vhost-user
library, really) know that this socket is orphaned. I guess, I'm saying
it's ****** that OVS is left holding the bag of deciding that it should
clean up.
I think if we are choosing to unlink though, this approach is the
sanest, frought with possible edge cases as it is. :)
>> What do you guys think?
>> ---
>> lib/netdev-dpdk.c | 5 +++++
>> 1 file changed, 5 insertions(+)
>>
>> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
>> index 6b33c02..e839eaa 100644
>> --- a/lib/netdev-dpdk.c
>> +++ b/lib/netdev-dpdk.c
>> @@ -694,6 +694,11 @@ netdev_dpdk_vhost_user_construct(struct netdev
>> *netdev_)
>> snprintf(netdev->vhost_id, sizeof(netdev->vhost_id), "%s/%s",
>> vhost_sock_dir, name);
>>
>> + if (unlink(netdev->vhost_id) && errno != ENOENT) {
>> + VLOG_WARN("unlinking \"%s\": %s\n",
>> + netdev->vhost_id, ovs_strerror(errno));
>> + }
>> +
>> err = rte_vhost_driver_register(netdev->vhost_id);
>> if (err) {
>> VLOG_ERR("vhost-user socket device setup failure for socket %s\n",
>> --
>> 2.1.4
>>
>> _______________________________________________
>> dev mailing list
>> dev@openvswitch.org
>> http://openvswitch.org/mailman/listinfo/dev
>>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
