> -----Original Message----- > From: David Miller [mailto:da...@davemloft.net] > Sent: Tuesday, January 31, 2017 9:51 AM > To: Grandhi, Sainath <sainath.gran...@intel.com> > Cc: netdev@vger.kernel.org; mah...@bandewar.net; linux- > ker...@vger.kernel.org > Subject: Re: [PATCHv3 5/7] tap: Extending tap device create/destroy APIs > > From: Sainath Grandhi <sainath.gran...@intel.com> > Date: Mon, 30 Jan 2017 11:12:00 -0800 > > > + list_for_each_entry_safe(tap_major, tmp, &major_list, next) { > > + if (tap_major->major == major) { > > + return tap_major; > > + } > > + } > > Single line basic blocks, such as this 'if' statement, should not be > surrounded > by curly braces. > > Also I see not mutual exclusion being implemented to protect this list. If > there > is, it is not obvious, so it should be documented with a comment explaining > what protects the list. Ok. It's my fault to not include safe reading of the list. I would use the RCU API for list traversal, adding and deleting entries. Please look for it in my next version.
> > > @@ -1175,14 +1228,7 @@ int tap_create_cdev(struct cdev *tap_cdev, > > if (err) > > goto out2; > > > > - macvtap_major.major = MAJOR(*tap_major); > > - > > - idr_init(&macvtap_major.minor_idr); > > - mutex_init(&macvtap_major.minor_lock); > > - > > - macvtap_major.device_name = device_name; > > - > > - return err; > > + return tap_list_add(*tap_major, device_name); > > > > out2: > > unregister_chrdev_region(*tap_major, TAP_NUM_DEVS); > > If tap_list_add() fails, you will leave the chrdev region registered. Thanks. Would take care of it.
