Virtualization - Apr 2021 - [virtio-comment] Re: [RFC PATCH v5 2/2] virtio-vsock: SOCK_SEQPACKET description

On Wed, Apr 21, 2021 at 06:09:21PM +0300, Arseny Krasnov
wrote:
>
>On 21.04.2021 12:54, Stefano Garzarella wrote:
>> On Wed, Apr 21, 2021 at 04:24:36AM -0400, Michael S. Tsirkin wrote:
>>> On Wed, Apr 21, 2021 at 09:45:23AM +0200, Stefano Garzarella wrote:
>>>> On Wed, Apr 14, 2021 at 09:04:47AM +0300, Arseny Krasnov wrote:
>>>>> On 13.04.2021 22:55, Michael S. Tsirkin wrote:
>>>>>> On Tue, Apr 13, 2021 at 05:22:44PM +0300, Arseny
Krasnov wrote:
>>>>>>> On 13.04.2021 16:10, Michael S. Tsirkin wrote:
>>>>>>>> On Tue, Apr 13, 2021 at 03:53:29PM +0300,
Arseny Krasnov wrote:
>>>>>>>>> This adds description of SOCK_SEQPACKET
socket type
>>>>>>>>> support for virtio-vsock.
>>>>>>>>>
>>>>>>>>> Signed-off-by: Arseny Krasnov
<arseny.krasnov at kaspersky.com>
>>>>>>>>> ---
>>>>>>>>>  virtio-vsock.tex | 26
+++++++++++++++++++++-----
>>>>>>>>>  1 file changed, 21 insertions(+), 5
deletions(-)
>>>>>>>>>
>>>>>>>>> diff --git a/virtio-vsock.tex
b/virtio-vsock.tex
>>>>>>>>> index ad57f9d..00e59cc 100644
>>>>>>>>> --- a/virtio-vsock.tex
>>>>>>>>> +++ b/virtio-vsock.tex
>>>>>>>>> @@ -16,7 +16,10 @@
\subsection{Virtqueues}\label{sec:Device Types / Socket Device / Virtqueues}
>>>>>>>>>
>>>>>>>>>  \subsection{Feature bits}\label{sec:Device
Types / Socket Device / Feature bits}
>>>>>>>>>
>>>>>>>>> -There are currently no feature bits
defined for this device.
>>>>>>>>> +\begin{description}
>>>>>>>>> +\item VIRTIO_VSOCK_F_SEQPACKET (0)
SOCK_SEQPACKET socket type is
>>>>>>>>> +    supported.
>>>>>>>> Does it make sense to only support seqpacket
and not stream?
>>>>>>>> I am guessing not since seqpacket is more or
less
>>>>>>>> a superset ...
>>>>>>> You mean, this sentence must be "Both
SOCK_SEQPACKET and SOCK_STREAM types
>>>>>>>
>>>>>>> are supported"?
>>>>>> No. I am asking whether we want a feature bit for
SOCK_STREAM too?
>>>>> I think? there is no practical sense in SOCK_STREAM bit,
because SOCK_SEQPACKET
>>>>>
>>>>> is stream + message boundaries and potential DGRAM is
completely different
>>>>>
>>>>> thing. Of course i can implement it in my patches and also
add it to spec patch, but? i see only
>>>>>
>>>>> esthetic in this: all three socket types have own feature
bits.
>>>>>
>>>> I agree that it may make sense to have a bit for SOCK_STREAM.
For example we
>>>> may have devices in the future that want to implement only
DGRAM for
>>>> simplicity.
>>>>
>>>> I'm just worried about backwards compatibility with current
devices where we
>>>> don't have any feature bit.
>>>>
>>>> Should we add a negative feature flag? (e.g.
VIRTIO_VSOCK_F_NO_STREAM)
>>>> I don't like it much, but I can't think of anything
better.
>>>>
>>>> Thanks,
>>>> Stefano
>>> We can simply specify that if there are no feature bits at all then
>>> stream is assumed supported.
>>>
>> oh yeah, that sounds like a good idea!
>
>So it is not necessary for my SEQPACKET patchset to support STREAM in both
>kernel and spec?
>
I don't think it's necessary for SEQPACKET, but I would reserve bit 0 to
stream.

We could add a patch to this series that adds the bit for stream and 
explains that if there is no feature bit set, then only stream is 
supported.

Or I can send it separately if you don't want to include it in the 
series.

Thanks,
Stefano