Hi, On Sat, Jul 04, 2015 at 01:42:31PM +0200, David Herrmann wrote: > Hi > > On Thu, Jul 2, 2015 at 3:45 PM, Sergei Zviagintsev <ser...@s15v.net> wrote: > > Hi David, > > > > Thank you for reviewing and providing comments on these all! I answered > > below. > > > > On Thu, Jul 02, 2015 at 10:50:47AM +0200, David Herrmann wrote: > >> Hi > >> > >> On Sun, Jun 28, 2015 at 3:17 PM, Sergei Zviagintsev <ser...@s15v.net> > >> wrote: > >> > 1) Rewrite > >> > > >> > quota->memory + memory > U32_MAX > >> > > >> > as > >> > U32_MAX - quota->memory < memory > >> > > >> > and provide the comment on why we need that check. > >> > > >> > We have no overflow issue in the original expression when size_t is > >> > 32-bit because the previous one (available - quota->memory < memory) > >> > guarantees that quota->memory + memory doesn't exceed `available' > >> > which is <= U32_MAX in that case. > >> > > >> > But lets stay explicit rather than implicit, it would save us from > >> > describing HOW the code works. > >> > > >> > 2) Add WARN_ON when quota->msgs > KDBUS_CONN_MAX_MSGS > >> > > >> > This is somewhat inconsistent, so we need to properly report it. > >> > >> I don't see the purpose of this WARN_ON(). Sure, ">" should never > >> happen, but that doesn't mean we have to add a WARN_ON. I'd just keep > >> the code as it is. > > > > I agree on WARN_ON. The intention of this change was to provide > > consistency. Current code checks for 'quota->msgs > KDBUS_CONN_MAX_MSGS' > > having '>=' test. If this ever happens, it means that we have a bug, but > > silently ignore it. > > > > If we agree that '>' case should never happen, isn't it better to > > place '==' instead of '>=' in the original test? > > I don't see why. This code does not care whether quota->msgs is bigger > than MAX_MSGS. Sure, it does not happen in current code, but this > code-path really doesn't care whether that case can happen or not. All > it does, it verify that it is smaller. Hence, we use ">=". > > Furthermore, I usually prefer being rather safe than sorry. WARN_ON()s > are usually not free, but ">=" is for free, if we already have a > condition.
ok, thank you for explanation! -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
