Re: [virtio-comment] Re: [RFC PATCH v1 0/1] virtio-vsock: introduce SEQPACKET description

From: Stefano Garzarella <sgarzare@redhat.com>
To: "Michael S. Tsirkin" <mst@redhat.com>
Cc: Andra Paraschiv <andraprs@amazon.com>,
	cohuck@redhat.com, Colin Ian King <colin.king@canonical.com>,
	oxffffaa@gmail.com, Norbert Slusarek <nslusarek@gmx.net>,
	Stefan Hajnoczi <stefanha@redhat.com>,
	virtio-comment@lists.oasis-open.org,
	Jakub Kicinski <kuba@kernel.org>,
	Arseny Krasnov <arseny.krasnov@kaspersky.com>,
	virtualization@lists.linux-foundation.org,
	"David S. Miller" <davem@davemloft.net>,
	Jorgen Hansen <jhansen@vmware.com>
Subject: Re: [virtio-comment] Re: [RFC PATCH v1 0/1] virtio-vsock: introduce SEQPACKET description
Date: Tue, 16 Mar 2021 15:08:22 +0100	[thread overview]
Message-ID: <20210316140822.gvdtqci22tlmh3x6@steredhat> (raw)
In-Reply-To: <20210316095002-mutt-send-email-mst@kernel.org>

On Tue, Mar 16, 2021 at 09:50:48AM -0400, Michael S. Tsirkin wrote:
>On Mon, Feb 22, 2021 at 11:16:54AM +0100, Stefano Garzarella wrote:
>> On Thu, Feb 18, 2021 at 09:07:12AM +0300, Arseny Krasnov wrote:
>> > This patchset adds description of SOCK_SEQPACKET socket's type
>> > of virtio vsock.
>> >
>> > Here is implementation:
>> > https://lkml.org/lkml/2021/2/18/24
>> >
>> > Arseny Krasnov(1):
>> >  virtio-vsock: add SOCK_SEQPACKET description
>> >
>> > virtio-vsock.tex | 40 +++++++++++++++++++++++++++++++++++++---
>> > 1 files changed, 37 insertions(+), 3 deletions(-)
>> >
>> > TODO:
>> > - for messages identification I use header's field called 'msg_cnt'.
>> >  May be it is better to call it 'msg_id' or 'msg_num', because it
>> >  is used as ID, but implemented as counter.
>>
>> If we use it only as an identifier, I think 'msg_id' is preferable and we
>> shouldn't mention in the specs that it's a counter, since it's just a
>> possible implementation.
>> Instead if we use the counter for some control, for example if we lost a
>> packet, then maybe msg_cnt is better.
>> But since the channel shouldn't lose packets, I don't think this is the
>> case.
>>
>> >
>> > - in current version of specification, some values of headers' fields
>> >  still unnamed, for example type of socket (stream or seqpacket), then
>> >  shutdown flags. Spec says that for stream socket value of 'type'
>> >  must be 1. For receive shutdown bit 0 is set in 'flags', for send
>> >  shutdown bit 1 is set in 'flags'. But in Linux these unnamed ones and
>> >  zeroes are implemented as enums, so may be it will be ok to add such
>> >  enums in specification (as 'enum virtio_vsock_event_id').
>>
>> Since we have an enumerate for VIRTIO_VSOCK_OP_* values, I think we can add
>> enums for socket type and maybe 'flags'.
>
>We really need to switch enums to defines, for consistency.

I fully agree, indeed at least in the virtio-vsock part, we use the 
enumerate as they were many defines, always redefining the assigned 
value.
Using defines, we are forced to always define the value and I think 
that's fair!

Thanks,
Stefano

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