netdev - Re: [RFC PATCH v2 00/13] virtio/vsock: introduce SOCK

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 18 Jan 2021 16:16:56 +0100
From:   Stefano Garzarella <sgarzare@...hat.com>
To:     Arseny Krasnov <arseny.krasnov@...persky.com>,
        stsp <stsp2@...dex.ru>
Cc:     Stefan Hajnoczi <stefanha@...hat.com>,
        "Michael S. Tsirkin" <mst@...hat.com>,
        Jason Wang <jasowang@...hat.com>,
        "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Colin Ian King <colin.king@...onical.com>,
        Andra Paraschiv <andraprs@...zon.com>,
        Jeff Vander Stoep <jeffv@...gle.com>,
        kvm <kvm@...r.kernel.org>,
        Linux Virtualization <virtualization@...ts.linux-foundation.org>,
        netdev <netdev@...r.kernel.org>,
        kernel list <linux-kernel@...r.kernel.org>,
        Krasnov Arseniy <oxffffaa@...il.com>
Subject: 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 пишет:
>>      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