Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1434525 > unrolled thread

Re: [PATCH v13 net-next 1/1] hv_sock: introduce Hyper-V Sockets

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v13 net-next 1/1] hv_sock: introduce Hyper-V Sockets David Miller <davem@davemloft.net> - 2016-06-30 15:00 +0200
    RE: [PATCH v13 net-next 1/1] hv_sock: introduce Hyper-V Sockets Dexuan Cui <decui@microsoft.com> - 2016-06-30 16:40 +0200

#1434525 — Re: [PATCH v13 net-next 1/1] hv_sock: introduce Hyper-V Sockets

From: Dexuan Cui <decui@microsoft.com>
Date: Wed, 29 Jun 2016 11:30:40 +0000

> @@ -1509,4 +1509,18 @@ static inline void commit_rd_index(struct vmbus_channel *channel)
>  }
>  
>  
> +struct vmpipe_proto_header {
> +	u32 pkt_type;

It is wasteful to have two empty lines before this structure definition, one
is sufficient.

> +/*
> + * This is the address fromat of Hyper-V Sockets.
> + * Note: here we just borrow the kernel's built-in type uuid_le. When
> + * an application calls bind() or connect(), the 2 members of struct
> + * sockaddr_hv must be of GUID.
> + * The GUID format differs from the UUID format only in the byte order of
> + * the first 3 fields. Refer to:
> + * https://en.wikipedia.org/wiki/Globally_unique_identifier
> + */

Comments should be of the form:

	/* Like
	 * this.
	 */

Rather than:

	/*
	 * Like
	 * this.
	 */

> +	__le16		reserved;	     /* Must be Zero		*/

Why does an ignored, reserved, field need an endianness?  Just use
plain "u16" for this.

> +static
> +void hvsock_enqueue_accept(struct sock *listener, struct sock *connected)

Don't split the declaration after "static" with a newline, instead use:

====================
static void hvsock_enqueue_accept(struct sock *listener,
				  struct sock *connected)
====================

> +{
> +	struct hvsock_sock *hvlistener;
> +	struct hvsock_sock *hvconnected;

Please order local variables from logest to shortest line.

> +static struct sock *hvsock_dequeue_accept(struct sock *listener)
> +{
> +	struct hvsock_sock *hvlistener;
> +	struct hvsock_sock *hvconnected;

Likewise.

> +static void hvsock_sk_destruct(struct sock *sk)
> +{
> +	struct hvsock_sock *hvsk = sk_to_hvsock(sk);
> +	struct vmbus_channel *channel = hvsk->channel;

Likewise.

> +/* This function runs in the tasklet context of process_chn_event() */
> +static void hvsock_on_channel_cb(void *ctx)
> +{
> +	struct sock *sk = (struct sock *)ctx;
> +	struct hvsock_sock *hvsk = sk_to_hvsock(sk);
> +	struct vmbus_channel *channel = hvsk->channel;
> +	bool can_read, can_write;

Likewise.

> +static int hvsock_open_connection(struct vmbus_channel *channel)
> +{
> +	struct hvsock_sock *hvsk, *new_hvsk;
> +	struct sockaddr_hv hv_addr;
> +	struct sock *sk, *new_sk;
> +	unsigned char conn_from_host;

Likewise.

> +static int hvsock_connect_wait(struct socket *sock,
> +			       int flags, int current_ret)
> +{
> +	struct sock *sk = sock->sk;
> +	struct hvsock_sock *hvsk = sk_to_hvsock(sk);

Likewise.

[toc] | [next] | [standalone]

#1434632

> From: David Miller [mailto:davem@davemloft.net]
> Sent: Thursday, June 30, 2016 20:45
> To: Dexuan Cui <decui@microsoft.com>
> Cc: gregkh@linuxfoundation.org; netdev@vger.kernel.org; linux-
> kernel@vger.kernel.org; devel@linuxdriverproject.org; olaf@aepfle.de;
> apw@canonical.com; jasowang@redhat.com; vkuznets@redhat.com;
> cavery@redhat.com; KY Srinivasan <kys@microsoft.com>; Haiyang Zhang
> <haiyangz@microsoft.com>; joe@perches.com; rolf.neugebauer@docker.com
> Subject: Re: [PATCH v13 net-next 1/1] hv_sock: introduce Hyper-V Sockets
> 
> From: Dexuan Cui <decui@microsoft.com>
> Date: Wed, 29 Jun 2016 11:30:40 +0000
> 
> > @@ -1509,4 +1509,18 @@ static inline void commit_rd_index(struct
> vmbus_channel *channel)
> >  }
> >
> >
> > +struct vmpipe_proto_header {
> > +	u32 pkt_type;
> 
> It is wasteful to have two empty lines before this structure definition, one
> is sufficient.
> 
> ...

Hi David,
Thank you for pointing out the issues!

I'll fix all of them, and check all the similar issues in the patch.

Will post a new version ASAP.

Thanks,
-- Dexuan

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web