Re: [PATCH2/4] fully enable rpctrace to trace multitask programs.

[olafBuddenhagen]

Hi,

On Mon, Jul 20, 2009 at 07:40:32PM +0800, Da Zheng wrote:

> 2009-07-20  Zheng Da  <zhengda1936@gmail.com>
> 
> fix bug #3939
> 
>       * rpctrace.c (traced_task): Relocate.
>       (wrap_all_threads): New function.
>       (wrap_new_thread): Likewise.
>       (trace_and_forward): Wrap all thread ports.
> 
> diff --git a/utils/rpctrace.c b/utils/rpctrace.c
> index b7379a7..f36e566 100644
> --- a/utils/rpctrace.c
> +++ b/utils/rpctrace.c
> @@ -82,6 +82,8 @@ msgid_ihash_cleanup (void *element, void *arg)
>  static struct hurd_ihash msgid_ihash
>    = HURD_IHASH_INITIALIZER (HURD_IHASH_NO_LOCP);
>  
> +task_t traced_task;
> +
>  /* Parse a file of RPC names and message IDs as output by mig's -list
>     option: "subsystem base-id routine n request-id reply-id".  Put each
>     request-id value into `msgid_ihash' with the routine name as its value.  
> */
> @@ -642,6 +644,80 @@ print_contents (mach_msg_header_t *inp,
>      }
>  }
>  
> +/* Wrap all thread port in the task */
> +static void
> +wrap_all_threads (task_t task)
> +{
> +  struct traced_info *thread_send_wrapper;
> +  thread_t *threads = NULL;
> +  size_t nthreads = 0;
> +  error_t err;
> +
> +  err = task_threads (task, &threads, &nthreads);
> +  if (err)

Seems rather pointless to initialize threads/nthreads, when the first
thing we do is setting them...

But that's a minor nitpick really :-)

> +    error (2, err, "task_threads");
> +
> +  for (int i = 0; i < nthreads; i++)

I realized only now that the existing rpctrace code uses prefix
increments rather than postfix increments everywhere... Please do the
same for consistency.

(Personally, I always found the prefix notation more intuitive -- no
idea why the other is much more popular :-) )

> +    {
> +      thread_send_wrapper = hurd_ihash_find (&traced_names, threads[i]);
> +      /* We haven't seen the port. */
> +      if (thread_send_wrapper == NULL)
> +     {
> +       mach_port_t new_thread_port;
> +       thread_send_wrapper = new_send_wrapper (threads[i], &new_thread_port);
> +       free (thread_send_wrapper->name);
> +       asprintf (&thread_send_wrapper->name, "thread%d", threads[i]);

Is the free() really necessary here? Seems odd...

(If it is, it deserves a comment.)

> +
> +       err = mach_port_insert_right (mach_task_self (),
> +                                     new_thread_port, new_thread_port,
> +                                     MACH_MSG_TYPE_MAKE_SEND);
> +       if (err)
> +         error (2, err, "mach_port_insert_right");
> +
> +       err = thread_set_kernel_port (threads[i], new_thread_port);

This could do with a comment I'd say -- if I didn't already know from
previous discussion, I'd probably have a hard time figuring out what it
is about...

> +       if (err)
> +         error (2, err, "thread_set_kernel_port");
> +
> +       mach_port_deallocate (mach_task_self (), new_thread_port);
> +     }
> +    }
> +  vm_deallocate (mach_task_self (), threads, nthreads * sizeof (thread_t));
> +}
> +
> +/* Wrap the new thread port that is in the message. */
> +static void
> +wrap_new_thread (mach_msg_header_t *inp)
> +{

A bit more explanation would be useful... What is the purpose of this
function, when does it get called (for which RPC), what does it actually
do?

I found out all that, but only from careful reading of the function
itself and the context it is called from. A comment explaining it would
definitely help with understanding the code.

> +  error_t err;
> +  mach_port_t thread_port;
> +  struct
> +    {
> +      mach_msg_header_t head;
> +      mach_msg_type_t retcode_type;
> +      kern_return_t retcode;
> +      mach_msg_type_t child_thread_type;
> +      mach_port_t child_thread;

child_thread is the receive end of the wrapper port for the new thread
port created in the RPC which' reply we are handling now, right?

Is there any particular reason you picked this name? Otherwise, you
might try to come up with something more self-explaining... Or at least
add a comment :-)

> +    } *reply = (void *) inp;
> +  /* This function is called after rewrite_right,
> +   * so the wrapper for the thread port has been created. */
> +  struct traced_info *send_wrapper = ports_lookup_port (traced_bucket,
> +                                                     reply->child_thread, 0);
> +
> +  assert (send_wrapper);
> +  err = mach_port_insert_right (mach_task_self (), reply->child_thread,
> +                             reply->child_thread, MACH_MSG_TYPE_MAKE_SEND);
> +  if (err)
> +    error (2, err, "mach_port_insert_right");
> +  thread_port = send_wrapper->forward;
> +  err = thread_set_kernel_port (thread_port, reply->child_thread);
> +  if (err)
> +    error (2, err, "thread_set_kernel_port");
> +  free (send_wrapper->name);
> +  asprintf (&send_wrapper->name, "thread%d", thread_port);
> +  mach_port_deallocate (mach_task_self (), reply->child_thread);

I think it would be clearer to drop it right after the
thread_set_kernel_port() call? It's created only to pass it in that
call, and we don't need our own copy anymore once we did that...

Also, probably better to move the thread_port assignment above the
mach_port_insert_right(), to make the grouping even more clear. (And
perhaps add some blank lines.)

BTW, it seems that I'm beginning to grok the reference counting stuff at
last :-)

> +  ports_port_deref (send_wrapper);
> +}
> +
>  int
>  trace_and_forward (mach_msg_header_t *inp, mach_msg_header_t *outp)
>  {
> @@ -770,6 +846,9 @@ trace_and_forward (mach_msg_header_t *inp, 
> mach_msg_header_t *outp)
>         putc (' ', ostream);
>         print_contents (&rh->Head, rh + 1);
>         putc ('\n', ostream);
> +
> +       if (inp->msgh_id == 2161)/* the reply message for thread_create */
> +         wrap_new_thread (inp);
>       }
>        else
>       {
> @@ -781,6 +860,11 @@ trace_and_forward (mach_msg_header_t *inp, 
> mach_msg_header_t *outp)
>         else
>           /* Leave a partial line that will be finished later.  */
>           fprintf (ostream, ")");
> +
> +       /* If it's the request of exec_startup_get_info,
> +        * it means that the traced process starts to run */
> +       if (inp->msgh_id == 30500)
> +         wrap_all_threads (traced_task);

You mentioned that you found a different way of handling this... I
assume it only applies when properly tracing multiple tasks?

>       }
>      }
>  
> @@ -1014,8 +1098,6 @@ print_data (mach_msg_type_name_t type,
>  
>  /*** Main program and child startup ***/
>  
> -task_t traced_task;
> -
>  
>  /* Run a child and have it do more or else `execvpe (argv, envp);'.  */
>  pid_t

The patch looks good otherwise :-)

-antrik-