On Fri, Jan 15, 2021 at 12:59:30PM +0300, stsp wrote: >15.01.2021 08:35, Arseny Krasnov пишет: >> 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 > > >> record length, when > >then > > >> 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
