Re: socket.c added support for unix domain socket datagram transport

[Ralph Schmieder]

> On Apr 23, 2021, at 18:54, Stefano Brivio <sbrivio@redhat.com> wrote:
> 
> On Fri, 23 Apr 2021 17:21:38 +0100
> Daniel P. Berrangé <berrange@redhat.com> wrote:
> 
>> On Fri, Apr 23, 2021 at 05:29:40PM +0200, Stefano Brivio wrote:
>>> Hi Ralph,
>>> 
>>> On Fri, 23 Apr 2021 08:56:48 +0200
>>> Ralph Schmieder <ralph.schmieder@gmail.com> wrote:
>>> 
>>>> Hey...  new to this list.  I was looking for a way to use Unix domain
>>>> sockets as a network transport between local VMs.
>>>> 
>>>> I'm part of a team where we run dozens if not hundreds of VMs on a
>>>> single compute instance which are highly interconnected.
>>>> 
>>>> In the current implementation, I use UDP sockets (e.g. something like 
>>>> 
>>>> -netdev
>>>> id=bla,type=socket,udp=localhost:1234,localaddr=localhost:5678) 
>>>> 
>>>> which works great.
>>>> 
>>>> The downside of this approach is that I need to keep track of all the
>>>> UDP ports in use and that there's a potential for clashes.  Clearly,
>>>> having Unix domain sockets where I could store the sockets in the
>>>> VM's directory would be much easier to manage.
>>>> 
>>>> However, even though there is some AF_UNIX support in net/socket.c,
>>>> it's
>>>> 
>>>> - not configurable
>>>> - it doesn't work  
>>> 
>>> I hate to say this, but I've been working on something very similar
>>> just in the past days, with the notable difference that I'm using
>>> stream-oriented AF_UNIX sockets instead of datagram-oriented.
>>> 
>>> I have a similar use case, and after some experiments I picked a
>>> stream-oriented socket over datagram-oriented because:
>>> 
>>> - overhead appears to be the same
>>> 
>>> - message boundaries make little sense here -- you already have a
>>>  32-bit vnet header with the message size defining the message
>>>  boundaries
>>> 
>>> - datagram-oriented AF_UNIX sockets are actually reliable and there's
>>>  no packet reordering on Linux, but this is not "formally" guaranteed
>>> 
>>> - it's helpful for me to know when a qemu instance disconnects for any
>>>  reason  
>> 
>> The current IP socket impl for the net socket backend uses SOCK_DGRAM,
>> so from a consistency POV it feels sensible todo the same for UNIX
>> sockets too.
> 
> That's just for UDP though -- it also supports TCP with the "connect="
> parameter, and given that a stream-oriented AF_UNIX socket behaves very
> similarly, I recycled that parameter and just extended that bit of
> documentation.
> 
>> None the less, your last point in particular about wanting to know
>> about disconnects feels valid, and if its useful to you for UNIX
>> sockets, then it ought to be useful for IP sockets too.
>> 
>> IOW, I wonder if  we should use DGRAM for UNIX sockets too by default
>> to match current behaviour, but then also add a CLI option that allows
>> choice of DGRAM vs STREAM, and wire that up for IP & UNIX sockets.
> 
> The choice would only apply to AF_UNIX (that is, not to TCP and UDP).
> 
> The current situation isn't entirely consistent, because for TCP you
> have "connect=", for UDP it's "udp=" or "mcast=", and I'm extending the
> "connect=" case to support stream-oriented AF_UNIX, which I think is
> consistent.
> 
> However, to have it symmetric for the datagram-oriented case
> (UDP and AF_UNIX), ideally it should be changed to
> "dgram=host:port|path" -- which I guess we can't do.

That's what I thought, too. It would be canonical but would break backwards 
compatibility... hence impossible to change the existing parameter from udp to 
dgram.

However, an alternative would be to use the dgram approach for both UDP and UDS 
dgram and keep the UDP option for backwards compatibility.

The code would have to ensure that the dgram param type matches the localaddr 
param type and that localaddr has to be present if dgram is provided (as with 
UDP currently).


> 
> I see two alternatives:
> 
> 1.
>  - "connect=" (TCP only)
>  - "unix=path,type=dgram|stream"
>  - "udp=" (UDP only)
> 
> 2.
>  - "connect=" (TCP and AF_UNIX stream)
>  - "unix_dgram="
>  - "udp=" (UDP only)
> 

> The major thing I like of 2. is that we save some code and a further
> option, but other than that I don't have a strong preference.
> 
> -- 
> Stefano