Re: [RFC PATCH v2 00/13] virtio/vsock: introduce SOCK_SEQPACKET support.

On Fri, Jan 15, 2021 at 12:59:30PM +0300, stsp wrote:

15.01.2021 08:35, Arseny Krasnov пишет:

quoted

     This patchset impelements support of SOCK_SEQPACKET for virtio
transport.
     As SOCK_SEQPACKET guarantees to save record boundaries, so to
do it, new packet operation was added: it marks start of record (with
record length in header), such packet doesn't carry any data.  To send
record, packet with start marker is sent first, then all data is sent
as usual 'RW' packets. On receiver's side, length of record is known
from packet with start record marker. Now as  packets of one socket
are not reordered neither on vsock nor on vhost transport layers, such
marker allows to restore original record on receiver's side. If user's
buffer is smaller that

than

quoted

 record length, when

then

quoted

 v1 -> v2:
 - patches reordered: af_vsock.c changes now before virtio vsock
 - patches reorganized: more small patches, where +/- are not mixed

If you did this because I asked, then this
is not what I asked. :)
You can't just add some static func in a
separate patch, as it will just produce the
compilation warning of an unused function.
I only asked to separate the refactoring from
the new code. I.e. if you move some code
block to a separate function, you shouldn't
split that into 2 patches, one that adds a
code block and another one that removes it.
It should be in one patch, so that it is clear
what was moved, and no new warnings are
introduced.
What I asked to separate, is the old code
moves with the new code additions. Such
things can definitely go in a separate patches.

Arseny, thanks for the v2.
I appreciated that you moved the af_vsock changes before the transport
and also the test, but I agree with stsp about split patches.

As stsp suggested, you can have some "preparation" patches that touch
the already existing code (e.g. rename vsock_stream_sendmsg in
vsock_connectible_sendmsg() and call it inside the new
vsock_stream_sendmsg, etc.), then a patch that adds seqpacket stuff in
af_vsock.

Also for virtio/vhost transports, you can have some patches that add
support in virtio_transport_common, then a patch that enable it in
virtio_transport and a patch for vhost_vsock, as you rightly did in
patch 12.

So, I'd suggest moving out the code that touches virtio_transport.c
from patch 11.

These changes should simplify the review.

In addition, you can also remove the . from the commit titles.


I left other comments in the single patches.

Thanks,
Stefano

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization