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

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] [day] [month] [year] [list]

Message-ID: <b50191f71b0f432593553cf23fab20ce@HKXPR3004MB0088.064d.mgd.msft.net>
Date:	Wed, 6 Jan 2016 05:27:17 +0000
From:	Dexuan Cui <decui@...rosoft.com>
To:	Vitaly Kuznetsov <vkuznets@...hat.com>
CC:	"gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>,
	"davem@...emloft.net" <davem@...emloft.net>,
	"stephen@...workplumber.org" <stephen@...workplumber.org>,
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"driverdev-devel@...uxdriverproject.org" 
	<driverdev-devel@...uxdriverproject.org>,
	"olaf@...fle.de" <olaf@...fle.de>,
	"apw@...onical.com" <apw@...onical.com>,
	"jasowang@...hat.com" <jasowang@...hat.com>,
	KY Srinivasan <kys@...rosoft.com>,
	"pebolle@...cali.nl" <pebolle@...cali.nl>,
	"stefanha@...hat.com" <stefanha@...hat.com>,
	"dan.carpenter@...cle.com" <dan.carpenter@...cle.com>
Subject: RE: [PATCH V5 9/9] hvsock: introduce Hyper-V VM Sockets feature

> From: Vitaly Kuznetsov [mailto:vkuznets@...hat.com]
> 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@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/