lkml.org 
[lkml]   [2013]   [Jan]   [28]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH 1/1] VSOCK: Introduce VM Sockets

Hi,

> diff --git a/net/vmw_vsock/Kconfig b/net/vmw_vsock/Kconfig
> new file mode 100644
> index 0000000..95e2568
> --- /dev/null
> +++ b/net/vmw_vsock/Kconfig
> @@ -0,0 +1,14 @@
> +#
> +# Vsock protocol
> +#
> +
> +config VMWARE_VSOCK
> + tristate "Virtual Socket protocol"
> + depends on VMWARE_VMCI

I guess this is temporary? Cover letter says *mostly* separated ...

> +vmw_vsock-y += af_vsock.o vmci_transport.o vmci_transport_notify.o \
> + vmci_transport_notify_qstate.o vsock_addr.o

Likewise, I expect with the final version vmci_transport is a separate
module (or moves into the vmci driver), correct?

> +static long vsock_dev_do_ioctl(struct file *filp,
> + unsigned int cmd, void __user *ptr)
> +{
> + static const u16 parts[4] = VSOCK_DRIVER_VERSION_PARTS;
> + u32 __user *p = ptr;
> + int retval = 0;
> + u32 version;
> +
> + switch (cmd) {
> + case IOCTL_VMCI_SOCKETS_VERSION:
> + version = VMCI_SOCKETS_MAKE_VERSION(parts);
> + if (put_user(version, p) != 0)
> + retval = -EFAULT;
> + break;

Still needed?

> + case IOCTL_VMCI_SOCKETS_GET_AF_VALUE:
> + if (put_user(AF_VSOCK, p) != 0)
> + retval = -EFAULT;
> +
> + break;

That can go away, with the upstream merge vsock will get a fixed AF_VSOCK.

> + case IOCTL_VMCI_SOCKETS_GET_LOCAL_CID:
> + if (put_user(vmci_get_context_id(), p) != 0)
> + retval = -EFAULT;

What is this?

> +static int __init vsock_init(void)
> +{
> + int err;
> +
> + vsock_init_tables();
> +
> + err = misc_register(&vsock_device);
> + if (err) {
> + pr_err("Failed to register misc device\n");
> + return -ENOENT;
> + }
> +
> + err = vmci_transport_register(&transport);
> + if (err) {
> + pr_err("Cannot register with VMCI device\n");
> + goto err_misc_deregister;
> + }

Hmm? There should be a vsock_(un)register_transport which the vmci
transport code can call (and likewise virtio transport some day).

> +struct vsock_sock {
> + /* sk must be the first member. */
> + struct sock sk;
> + struct sockaddr_vm local_addr;
> + struct sockaddr_vm remote_addr;

> + /* The rest is transport-specific: this is the stuff we need to pull
> + * out to make it work with something other than VMCI.
> + */
> + struct {
> + /* For DGRAMs. */
> + struct vmci_handle dg_handle;

Yep, should be a pointer where transports can hook in their private data.

> +/**** TRANSPORT ****/
> +
> +struct vsock_transport {
> + void (*init)(struct vsock_sock *, struct vsock_sock *);
> + void (*destruct)(struct vsock_sock *);
> + void (*release)(struct vsock_sock *);
> + int (*connect)(struct vsock_sock *);
> + int (*bind_dgram)(struct vsock_sock *, struct sockaddr_vm *);
> + int (*send_dgram)(struct vsock_sock *, struct sockaddr_vm *,
> + struct iovec *, size_t len);
> + ssize_t (*recv_stream)(struct vsock_sock *, struct iovec *,
> + size_t len, int flags);
> + ssize_t (*send_stream)(struct vsock_sock *, struct iovec *,
> + size_t len);
> + s64 (*stream_has_data)(struct vsock_sock *);
> + s64 (*stream_has_space)(struct vsock_sock *);
> + int (*send_shutdown)(struct sock *sk, int mode);
> + void (*unregister)(void);
> +};

So that is the interface transports have to implement. Looks reasonable
to me. Some documentation would be nice, although most of it is
self-explaining.

Where is recv_dgram?

Also why bind_dgram? I guess binding stream sockets doesn't make sense
for the vsock family?

I'd make the naming a bit more consistent, some stream callbacks are
prefixed and some postfixed with "stream".

I'd also name send_shutdown just shutdown (same name the system call has).

What does unregister?

cheers,
Gerd


\
 
 \ /
  Last update: 2013-01-28 14:21    [W:0.135 / U:1.216 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site