[prev in list] [next in list] [prev in thread] [next in thread] 

List:       haiku-commits
Subject:    [haiku-commits] Re: haiku: hrev48748 - src/servers/app/decorator
From:       John Scipione <jscipione () gmail ! com>
Date:       2015-01-30 21:21:43
Message-ID: 54CBF5E7.40904 () gmail ! com
[Download RAW message or body]

Adrien Destugues wrote:
> On Fri, Jan 30, 2015 at 10:54:33AM -0500, John Scipione wrote:
> > So what is the solution here? The assert only checks that the list count is
> > > 0. What if the list has a NULL hole in it as in:
> > Tab 1
> > Tab 2
> > NULL
> > Tab3
> > 
> > ??
> 
> This should be checked when inserting items to the list, not when
> iterating it. This way the bug is detected immediately instead of
> at an unrelated point in the code.
> 
> If all insertions are checked against this, and it still happens, it
> means the list is getting corrupted. In that case, it's better to
> crash (and having no NULL check will do just that).
> 
> > > For example, if another thread is modifying the value, such a check like
> > > this could potentially mask the need for locking.
> > > 
> > I wasn't really thinking about thread locking issues although that is
> > another good point. Would a solution that locks access to the list before
> > modifying it be overkill here?
> 
> I mentioned threading issues in the ticket already. I will repeat what
> I said in my message:
> - If this function must only be called with some lock held, that should
> be documented in a comment for the function. This way we know what to
> expect. It could also check that the lock is actually held with an
> assert (for example BView does a lot of "looper must be locked"
> checks and will call debugger in case of misuses).
> - If this function can be called without any lock held, then we must
> review the code to see if the list can be accessed by multiple
> threads. If so, we must add locking.
> 
> It's not possible to decide wether locking should be added without
> further study of the code.

I'm sorry I must have misunderstood what you wrote on #11111. I thought that when you \
wrote "Then, I would also add a NULL check on the tab inside the loop (and skip the \
tab if it is NULL)" you meant that you thought we should test if the tab was NULL \
inside the loop and skip it in that case as this patch does.

This problem seems like a great opportunity for a newcomer to make the decorator \
system more robust and gain some experience with Haiku's API. I would like to create \
a new ticket with these details in mind so that the issue can be claimed by an \
interested developer. Do you (Adrian) mind if I use your above description in order \
to create the bug report?


[prev in list] [next in list] [prev in thread] [next in thread]