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

[Arseny Krasnov <arseny.krasnov@kaspersky.com>]

On 22.04.2021 12:00, Stefano Garzarella wrote:
> 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@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.

I can use bit 1 for SEQPACKET, keeping bit 0 free for STREAM, but

i'd like to exclude this from patchset, to make it smaller

>
> Thanks,
> Stefano
>
>