Re: [RFC] kern: simple futex for gnumach (version 11)

[Richard Braun]

On Thu, Jan 09, 2014 at 02:36:18PM -0200, Diego Nieto Cid wrote:
> 2014/1/8 Marin Ramesa <mpr@hi.t-com.hr>
> >
> > +
> > +static unsigned long futex_init(task_t task, vm_offset_t address, 
> > boolean_t private_futex, struct futex *futex)
> > +{
> > +       unsigned long node_slot = 0;
> > +
> > +       futex = (struct futex *)kalloc(sizeof(*futex));
> 
> This only changes the value of the parameter. "futex" could very well
> be a local variable instead.
> 
> If what you want is to update the value seen by the caller you should
> declare futex as a "struct futex **" and then do something like this:
> 
>     *futex = (struct futex *)kalloc(sizeof(**futex));

Right, this was already mentioned in an earlier review, and is still
there. Fix it.

> > +void futex_wait(task_t task, vm_offset_t address, int value, int /* TODO 
> > Use time_value_t */ msec, boolean_t private_futex)
> > +{
> > +       unsigned long node_slot = 0;
> > +
> > +       node_slot = futex_lookup_address(address);
> > +       if (node_slot == 0) {
> > +               if (private_futex) {
> > +                       pfutexes = (struct futex *)kalloc(sizeof(pfutexes));
> > +                       if (pfutexes == NULL)
> > +                               return;
> > +                       node_slot = futex_init(task, address, TRUE, 
> > &pfutexes[ARRAY_SIZE(pfutexes) - 1]);
> 
> I don't really get this stuff.
> 
> First of all, you are allocating "sizeof(pfutexes)" and pfutexes is
> defined as "struct futex *". Thus, you are just allocating memory for
> a pointer (4 bytes in a 32-bit machine) and then casting it to a
> pointer to "struct futex". So, if you dereference pfutexes to access
> some member of the futex structure you will probably go past the end
> of the allocated space (for the futex structure is bigger than a
> pointer).

I agree. this array isn't needed, get rid of it.

> Secondly, you are calling the macro ARRAY_SIZE with pfutexes as
> parameter. This will be evaluated to 0. Here's why:

Simply put, ARRAY_SIZE() is intended for statically allocated arrays
where the size is known at compile time. Don't use it on pointers.
But again, since there shouldn't be an array in the first place, this
function should be entirely reworked.

-- 
Richard Braun