RE: [PATCH net-next, 2/4] hv_netvsc: Fix error handling in netvsc_attach()

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




> -----Original Message-----
> From: Markus Elfring <Markus.Elfring@xxxxxx>
> Sent: Friday, November 1, 2019 4:43 PM
> To: Haiyang Zhang <haiyangz@xxxxxxxxxxxxx>; linux-
> hyperv@xxxxxxxxxxxxxxx; netdev@xxxxxxxxxxxxxxx
> Cc: kernel-janitors@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; David S.
> Miller <davem@xxxxxxxxxxxxx>; KY Srinivasan <kys@xxxxxxxxxxxxx>; Olaf
> Hering <olaf@xxxxxxxxx>; Sasha Levin <sashal@xxxxxxxxxx>; Stephen
> Hemminger <sthemmin@xxxxxxxxxxxxx>; vkuznets <vkuznets@xxxxxxxxxx>
> Subject: Re: [PATCH net-next, 2/4] hv_netvsc: Fix error handling in
> netvsc_attach()
> 
> > If rndis_filter_open() fails, we need to remove the rndis device
> > created in earlier steps, before returning an error code. Otherwise,
> > the retry of
> > netvsc_attach() from its callers will fail and hang.
> 
> How do you think about to choose a more “imperative mood” for your
> change description?
> https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgit.k
> ernel.org%2Fpub%2Fscm%2Flinux%2Fkernel%2Fgit%2Ftorvalds%2Flinux.git%
> 2Ftree%2FDocumentation%2Fprocess%2Fsubmitting-
> patches.rst%3Fid%3D0dbe6cb8f7e05bc9611602ef45980a6c57b245a3%23n151
> &amp;data=02%7C01%7Chaiyangz%40microsoft.com%7C162aa016f45e4293c
> abb08d75f0c0fee%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637
> 082377796295159&amp;sdata=ytjxGYTPI2D4BoNbslKPvBbsfGUEM7hXj1YAiG
> hn8Ik%3D&amp;reserved=0
Agreed. Thanks.


> 
> 
> …
> > +++ b/drivers/net/hyperv/netvsc_drv.c
> > @@ -982,7 +982,7 @@ static int netvsc_attach(struct net_device *ndev,
> >  	if (netif_running(ndev)) {
> >  		ret = rndis_filter_open(nvdev);
> >  		if (ret)
> > -			return ret;
> > +			goto err;
> >
> >  		rdev = nvdev->extension;
> >  		if (!rdev->link_state)
> …
> 
> I would prefer to specify the completed exception handling (addition of two
> function calls) by a compound statement in the shown if branch directly.
> 
> If you would insist to use a goto statement, I find an other label more
> appropriate according to the Linux coding style.

I will have more patches that make multiple entry points of error clean up 
steps -- so I used goto instead of putting the functions in each if-branch.

I will name the labels more meaningfully.

Thanks,
- Haiyang




[Index of Archives]     [Kernel Development]     [Kernel Announce]     [Kernel Newbies]     [Linux Networking Development]     [Share Photos]     [IDE]     [Security]     [Git]     [Netfilter]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Device Mapper]

  Powered by Linux