RE: [PATCH V5 9/9] hvsock: introduce Hyper-V VM Sockets feature

[Dexuan Cui]

> From: Vitaly Kuznetsov [mailto:vkuznets@xxxxxxxxxx]
> Sent: Tuesday, January 5, 2016 17:52
> 
> Just some minor nitpicks below -- I have to admit I didn't test the feature.
> 
> [..skip..]
> 
> > +
> > +	if (sk->sk_err) {
> > +		ret = -sk->sk_err;
> > +		goto out_wait_error;
> > +	} else {
> > +		ret = 0;
> > +	}
> > +
> > +out_wait:
> > +	finish_wait(sk_sleep(sk), &wait);
> > +out:
> > +	release_sock(sk);
> > +	return ret;
> > +
> > +out_wait_error:
> > +	sk->sk_state = SS_UNCONNECTED;
> > +	sock->state = SS_UNCONNECTED;
> > +	goto out_wait;
> > +}
> 
> Why not just place out_wait_error label before out_wait (and do 'goto
> out_wait' in ret = 0 case instead of 'goto out_wait_error' in the error
> case)?
Good point.  I'll update the code.

> [..skip..]
> 
> > +
> > +static int __init hvsock_init(void)
> > +{
> > +	int ret;
> > +
> > +	/* Hyper-V socket requires at least VMBus 4.0 */
> > +	if ((vmbus_proto_version >> 16) < 4) {
> > +		pr_err("failed to load: VMBus 4 or later is required\n");
> > +		return -ENODEV;
> 
> (Let me pretend I'm Dan :-) So here we return ...
> 
> > +	}
> > +
> > +	ret = vmbus_driver_register(&hvsock_drv);
> > +	if (ret) {
> > +		pr_err("failed to register hv_sock driver\n");
> > +		goto out;
> 
> ... and here we goto where we just return. I suggest we bring some
> consistency by directly returning ret here and eliminating 'out' label.

Thanks! I'll update the code.

> > +	}
> > +
> > +	ret = proto_register(&hvsock_proto, 0);
> > +	if (ret) {
> > +		pr_err("failed to register protocol\n");
> > +		goto unreg_hvsock_drv;
> > +	}
> > +
> > +	ret = sock_register(&hvsock_family_ops);
> > +	if (ret) {
> > +		pr_err("failed to register address family\n");
> > +		goto unreg_proto;
> > +	}
> > +
> > +	return 0;
> > +
> > +unreg_proto:
> > +	proto_unregister(&hvsock_proto);
> > +unreg_hvsock_drv:
> > +	vmbus_driver_unregister(&hvsock_drv);
> > +out:
> > +	return ret;
> > +}
> > +
> > +static void __exit hvsock_exit(void)
> > +{
> > +	sock_unregister(AF_HYPERV);
> > +	proto_unregister(&hvsock_proto);
> > +	vmbus_driver_unregister(&hvsock_drv);
> > +}
> > +
> > +module_init(hvsock_init);
> > +module_exit(hvsock_exit);
> > +
> > +MODULE_DESCRIPTION("Microsoft Hyper-V Virtual Socket Family");
> > +MODULE_VERSION("0.1");
> 
> Do we really need it? When the driver is commited we won't probably be
> updating it with v0.2 as a whole, we'll be sending patches addressing
> issues and there always will be a question when to swtich to 0.2, 0.3,
> ... And we don't have MODULE_VERSION for other Hyper-V drivers.

Good point. I'll remove the line MODULE_VERSION.
 
Thanks,
-- Dexuan
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/