[haiku-commits] Re: r34395 - in haiku/trunk/src/libs/compat/freebsd_network: . compat/sys
- From: Ingo Weinhold <ingo_weinhold@xxxxxx>
- To: haiku-commits@xxxxxxxxxxxxx
- Date: Tue, 01 Dec 2009 17:12:26 +0100
On 2009-12-01 at 15:51:56 [+0100], Colin Günther <coling@xxxxxx> wrote:
> Ingo Weinhold schrieb:
> > On 2009-11-30 at 23:27:02 [+0100], coling@xxxxxx wrote:
> >
> >> Author: colin
> >> Date: 2009-11-30 23:27:02 +0100 (Mon, 30 Nov 2009)
> >> New Revision: 34395
> >> Changeset: http://dev.haiku-os.org/changeset/34395/haiku
> >>
> > [...]
> >
> >> Modified:
> >> haiku/trunk/src/libs/compat/freebsd_network/compat/sys/condvar.h
> >> ===================================================================
> >> --- haiku/trunk/src/libs/compat/freebsd_network/compat/sys/condvar.h
> >> 2009-11-30 22:07:16 UTC (rev 34394)
> >> +++ haiku/trunk/src/libs/compat/freebsd_network/compat/sys/condvar.h
> >> 2009-11-30 22:27:02 UTC (rev 34395)
> >> @@ -10,7 +10,7 @@
> >>
> >>
> >> struct cv {
> >> - int cv_waiters;
> >> + int dummy;
> >> };
> >>
> >
> > It's still not much clearer to me, why you use this structure.
> The "struct cv" is needed to stay source compatible with FreeBSD wlan
> stack. For example
> http://dev.haiku-os.org/browser/haiku/trunk/src/libs/compat/freebsd_wlan/net80211/ieee80211_scan.c#L62
> this structure is defined and used throughout this file.
I didn't mean the structure as such, but how it is defined now. The first
version, containing the ConditionVariable pointer seemed to be the most
useful one IMO.
> > Unless you
> > really need uninit_condition_variables() to destroy condition variables
--
> > which doesn't look like it from what I see --
> Mmh, so I thought
>
> while (variable != NULL) {
> ConditionVariable* next = definition.GetLink(variable);
> variable->Unpublish();
> delete variable;
>
>
> in uninit_condition_variables() destroys the ConditionVariables. Do I
> miss something here?
It does (ignoring the fact that you still use Unpublish() on condition
variables that haven't been Publish()ed in the first place), but that is not
my point. The question is: Are there any cv objects in the has at this point
at all? The way you create you use the in msleep() and friends, they are
freed anyway. And usually, particularly in kernelland, one explicitly frees
resources one has allocated, so that there shouldn't be any leftovers.
> > I would simply get rid of the
> > hash table, add the ConditionVariable pointer back to the cv structure
> > (well, you could as well get rid of the structure as well
> I'd like to get rid of the hash table and the tracking of used
> ConditionVariables, too. But the functions in Condvar.cpp are intended
> to support two use cases:
> 1) Implementing FreeBSDs condition variable subsystem (this is the one
> which needs the "struct cv")
> 2) Implementing FreeBSDs synchronization subsystem
> (http://dev.haiku-os.org/browser/haiku/trunk/src/libs/compat/freebsd_network
> /synch.c)
>
> The hash table is really needed for Use Case 2) function wakeup. So that
> I can retrieve the ConditionVariable belonging to this identifier.
That's why I suggested to introduce the static ConditionVariable::Notify*()
methods. There's really no need to access the ConditionVariable object when
you only want to notify the condition.
> > and just use
> > ConditionVariable pointers), and add static versions of the
> > ConditionVariable::Notify{One,All}() methods that take the identifying
> > object as first parameter.
> >
> Do you mean I should add static versions of
> ConditionVariable::Notify{One,All}() methods to Haiku's
> condition_variable.{h,cpp} ? I don't see such versions in the current
> source, at least.
Exactly.
CU, Ingo
Other related posts:
