Re: NTDLL: implement NtSignalAndWaitForSingleObject

[Robert Shearman]

Mike McCormack wrote:
> ChangeLog:
> - implement NtSignalAndWaitForSingleObject
> Index: server/thread.c
>
> RCS file: /home/wine/wine/server/thread.c,v
> retrieving revision 1.110
> diff -u -p -r1.110 thread.c
> --- server/thread.c	4 Mar 2005 12:38:36 -0000	1.110
> +++ server/thread.c	18 Apr 2005 16:28:02 -0000
> @@ -577,6 +577,73 @@ void wake_up( struct object *obj, int ma
>     }
> }
>
> +/*
> -
> - signal_object
> -
> -
> -
> - Try signaling an event flag, a semaphore or a mutex.
> -
> - FIXME: find out which operation to do by checking the type of object
> - */
>
> +static void signal_object( obj_handle_t handle )
> +{
> - /* is it an event flag? */
> - set_error( STATUS_SUCCESS );
> - event_operation( handle, SET_EVENT );
> - if (get_error() != STATUS_OBJECT_TYPE_MISMATCH)
> -
>    return;
> -
> - /* is it a semaphore? */
> - set_error( STATUS_SUCCESS );
> - release_semaphore( handle, 1, NULL );
> - if (get_error() != STATUS_OBJECT_TYPE_MISMATCH)
> -
>    return;
> -
> - /* is it a mutex? */
> - set_error( STATUS_SUCCESS );
> - release_mutex( handle, NULL );
> - if (get_error() != STATUS_OBJECT_TYPE_MISMATCH)
> -
>    return;
> -
> - set_error( STATUS_INVALID_HANDLE );
>
> +}

I think this is a little ugly. I'd prefer to see this code do a switch 
on obj->ops and call the relevant function with the raw object instead 
of a handle.
> -
>
> +static void signal_and_wait( obj_handle_t hsignal, struct object *wait,
> -
>                        int flags, const abs_time_t *timeout )
>
> +{
> - int r;
> -
> - if (!wait_on( 1, &wait, flags, timeout ))
> -
>    return;
> -
> - /* signal the object */
> - signal_object( hsignal );
> - if (get_error())
> - {
> -
>    end_wait( current );
> -
>    return;
> - }
> -
> - /* check if we woke ourselves up */
> - if (!current->wait)
> -
>    return;
> -
> - r = check_wait( current );
> - if (r != -1)
> - {
> -
>    end_wait( current );
> -
>    set_error( r );
> -
>    return;
> - }
> -
> - /* check if we need to wait */
> - if (!(flags & SELECT_TIMEOUT))
> -
>    return;
> -
> - current->wait->user = add_timeout_user( &current->wait->timeout,
> -
>                                        thread_timeout, current->wait );
> - if (!current->wait->user)
> -
>    end_wait( current );
>
> +}
> -
>
> /* queue an async procedure call */
> int thread_queue_apc( struct thread *thread, struct object *owner, void *func,
>                       enum apc_type type, int system, void *arg1, void *arg2, void *arg3 )

This seems to be pretty similar to select_on. Would it not be possible 
to use that function instead of duplicating that code?

Rob

NTDLL: implement NtSignalAndWaitForSingleObject

[Mike McCormack]

> I think this is a little ugly. I'd prefer to see this code do a switch 
> on obj->ops and call the relevant function with the raw object instead 
> of a handle.

I know it's ugly; I rewrote that bit three times, including once with an 
ops->signal() method.  Implementing a object operation requires that the 
3 operations are consistent.  For example, the access flags required to 
perform must be consistent, so the get_handle_obj can be done only once. 
  Unfortunately, semaphores and event flags require different 
permissions to execute ops->signal().

See conversation I had with Julliard below.
> This seems to be pretty similar to select_on. Would it not be possible 
> to use that function instead of duplicating that code?

I considered that.  The problem is that we would have to signal the 
object in the middle of the setup for select_on().  That means passing 
new parameters to select_on(), and that function already looks a little 
busy.

Mike

<mike_m> julliard: i have a dilemma implementing SignalObjectAndWait
<mike_m> the object to signal can be a semaphore, event flag, etc
<mike_m> i get a handle
<mike_m> so i have to resolve the object type by dereferencing the handle
<mike_m> however, different objects require different permissions
<mike_m> so a semaphore requires SEMAPHORE_MODIFY_STATE but a mutex does not
<mike_m> so in the object->signal method, i'm going to pass an 
obj_handle_t instead of a struct object
<mike_m> which is a little bit inconsistent
<mike_m> is this still consistent with implementing an obj->ops->signal() ?
<mike_m> esp. if it is obj->ops->signal( obj_handle_t ) ?
<mike_m> or should i extract the handle's permissions and pass that to 
signal( struct object *obj, DWORD perms ) ?
<mike_m> (you were right that this function is strangely designed)
<julliard> mike_m: passing a handle to an object function is not good
<mike_m> yeah.
<julliard> i guess you'll need to do it by hand
<mike_m> you mean without the function pointer
<mike_m> without an obj->ops->signal function
<julliard> yes

NTDLL: implement NtSignalAndWaitForSingleObject

[Robert Shearman]

Mike McCormack wrote:
> > I think this is a little ugly. I'd prefer to see this code do a 
> > switch on obj->ops and call the relevant function with the raw object 
> > instead of a handle.
>
> I know it's ugly; I rewrote that bit three times, including once with 
> an ops->signal() method.  Implementing a object operation requires 
> that the 3 operations are consistent.  For example, the access flags 
> required to perform must be consistent, so the get_handle_obj can be 
> done only once.  Unfortunately, semaphores and event flags require 
> different permissions to execute ops->signal().

Ok, I see the problem now. The only alternative I can think of is to try 
to open the object with the GENERIC_WRITE access right and implement 
generic mappings for each object (these mappings are really needed 
anyway for other situaitons).
> > This seems to be pretty similar to select_on. Would it not be 
> > possible to use that function instead of duplicating that code?
>
> I considered that.  The problem is that we would have to signal the 
> object in the middle of the setup for select_on().  That means passing 
> new parameters to select_on(), and that function already looks a 
> little busy.

You could refactor the function so that the wait_on bit is separate. 
That seems to be the only thing you have to do before signalling the object.

Rob

NTDLL: implement NtSignalAndWaitForSingleObject

[Robert Shearman]

Robert Shearman wrote:
> Mike McCormack wrote:
> > > I think this is a little ugly. I'd prefer to see this code do a 
> > > switch on obj->ops and call the relevant function with the raw 
> > > object instead of a handle.
> >
> > I know it's ugly; I rewrote that bit three times, including once with 
> > an ops->signal() method.  Implementing a object operation requires 
> > that the 3 operations are consistent.  For example, the access flags 
> > required to perform must be consistent, so the get_handle_obj can be 
> > done only once.  Unfortunately, semaphores and event flags require 
> > different permissions to execute ops->signal().
>
> Ok, I see the problem now. The only alternative I can think of is to 
> try to open the object with the GENERIC_WRITE access right and 
> implement generic mappings for each object (these mappings are really 
> needed anyway for other situaitons).

Actually, I just thought about one more solution. You could open the 
object with using an access right of 0, meaning that anything is 
accepted, then check the access right manually. Still a bit of a kludge, 
but I think it is better than the brute force chaining.

Rob

NTDLL: implement NtSignalAndWaitForSingleObject

[Mike McCormack]

> Actually, I just thought about one more solution. You could open the 
> object with using an access right of 0, meaning that anything is 
> accepted, then check the access right manually. Still a bit of a kludge, 
> but I think it is better than the brute force chaining.

Yeah, I'm not really happy with it the way it is either, thus the FIXME. 
   For now, it works and demonstrates the problem with 
SignalObjectAndWait...  I'm interested to hear Alexandre's opinion 
before trying to fix it myself.

Mike