Tue, Aug 14, 2012 at 03:14:00PM CEST, fbl@xxxxxxxxxx wrote: >On Tue, 14 Aug 2012 14:24:33 +0200 >Jiri Pirko <jiri@xxxxxxxxxxx> wrote: > >> Mon, Aug 13, 2012 at 07:52:17PM CEST, fbl@xxxxxxxxxx wrote: >> >On Mon, 13 Aug 2012 17:27:00 +0200 >> >Jiri Pirko <jiri@xxxxxxxxxxx> wrote: >> >> + /* >> >> + * To prevent loops, check if dev is not upper device to upper_dev. >> >> + */ >> >> + if (__netdev_has_upper_dev(upper_dev, dev, true)) >> >> + return -EBUSY; >> >> + >> >> + if (__netdev_find_upper(dev, upper_dev)) >> >> + return -EEXIST; >> > >> >__netdev_has_upper_dev() can go all the way up finding the device and >> >the __netdev_find_upper() just check the first level. >> >> >> I do not think this ordering is somewhat inportant. > >it's not the order, see below: > >> >I think it would be better to use: >> >__netdev_find_upper_dev(,,deep=true/false) >> >__netdev_has_upper(,) > >It's their names. Currently, the function ..._find_... look at >one level only, while the function ..._has_... does one or more >levels. I think it's better to swap 'has' and 'find' in their names: > >__netdev_find_upper_dev(,,deep=true/false) <-- find in all levels >__netdev_has_upper(,) <-- check only the one level. Oh, now I think I see your point. But realise this: The main reason for __netdev_find_upper() is to find "struct upper" for netdev_upper_dev_unlink(). Therefore the name is not "__netdev_find_upper_dev" and there's no need to go deep here. On the orher hand, __netdev_has_upper_dev() only says whether device is lower to specified upper device. In this case I think the name is quite convenient as well. > >fbl
