> From: Vitaly Kuznetsov [mailto:vkuzn...@redhat.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
_______________________________________________
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
