Re: [PATCH v11 0/3] virtio-vsock: SOCK_SEQPACKET description

[Stefano Garzarella <sgarzare@redhat.com>]

On Thu, Jan 13, 2022 at 11:50:09AM +0100, Cornelia Huck wrote:
> On Wed, Jan 12 2022, Stefano Garzarella <sgarzare@redhat.com> wrote:
>
> > v11:
> > - reworked "Message and record boundaries" paragraph [stefanha]
> >
> > v10: https://markmail.org/message/aebu4zqp4kxdm4ej
> >
> > Linux kernel and QEMU already merged SOCK_SEQPACKET support,
> > so I'm resending Arseny's patches to have consistent virtio-spec
> > and implementation.
>
> It really should not have been merged without a spec :( But now that it

Yeah, I agree but the linux-net maintainers merged it right away, so we 
rushed to merge QEMU too to use the feature.

> has happened already, we need to get a proper spec added sooner rather
> than later, and it would be nice if it could make virtio 1.2, which
> would mean that voting needs to start this week.

Having it in virtio 1.2 would be great.

> (I don't see an issue at https://github.com/oasis-tcs/virtio-spec/issues
> yet; creating one would be an obvious pre-req.)

Do I create it now or when we have all the patches ready?

> > I added patch 2, following the discussion about F_STREAM feature bit:
> > https://markmail.org/message/aoaspjy2jhidwbuo#query:+page:1+mid:obw54zzikgqimhjk+state:results
> >
> > About patch 2, the vhost-vsock device in the Linux kernel doesn't set bit 0
> > (F_STREAM), so at this point I don't know if it's better to use a negative
> > feature flag (e.g. F_NO_STREAM) or we go for F_STREAM and send a patch to
> > linux-stable (and QEMU?) to solve this issue.
>
> I fear that even with a patch for stable, there can still be old setups
> around for which "only F_SEQPACKET set" translates to "supports both
> stream and seqpacket"...

Which is usually true, since seqpacket is pretty much stream with the 
message/record boundaries.

> so I guess it mostly becomes a question of
> whether ignoring those broken setups is tolerable. (The old setups not
> setting any feature bits are fine with the proposed patch.) Using a
> negative feature bit is uglier, but also clearer. I suppose we want to
> be able to make stream sockets optional?

Yes, especially with other types, like datagram (not yet merged). We 
would like to be able to allow just datagram, without stream and 
seqpacket.

> If so, I think the negative
> approach is the better option, unless we can live with some broken
> setups.

Yes, it's probably the best thing.

I'm thinking about QEMU and the mess to add a new kernel supported 
feature. (vsock is almost always used with vhost-vsock).
We might be in the case where we migrate from a new QEMU that supports 
F_STREAM to an old one that doesn't set it but supports stream anyway.

So I'd like to change patch 2 to F_NO_STREAM. For the spec, it's not the 
best, but for the current implementation, I think it's the cleanest 
solution.
Otherwise maybe we should write in the spec that if F_SEQPACKET is set 
this means that stream is supported even if F_STREAM is not set.


Michael, Stefan, what do you think?


Thanks,
Stefano