[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH4/4] fully enable rpctrace to trace multitask programs.
|
From: |
olafBuddenhagen |
|
Subject: |
Re: [PATCH4/4] fully enable rpctrace to trace multitask programs. |
|
Date: |
Sat, 24 Oct 2009 03:23:23 +0200 |
|
User-agent: |
Mutt/1.5.19 (2009-01-05) |
Hi,
Sorry for being so late with this... It's really hard to motivate myself
to review such a large patch, knowing how long it will take me...
Hope you haven't forgotten everything about it in the meantime :-)
As always, my comments are mostly about formalities. I hope my
perfectionism doesn't discourage you from sending further patches :-)
(Strictly speaking, these cleanups could be done in extra patches; and
in fact it probably makes sense to put them in extra commits in your
personal repository, while you are testing stuff. In the ultimate patch
series you submit for review and merging into master however, it's
better to have them squashed into the main patch, so that any changes
compared to the original rpctrace are as clear as possible...)
On Mon, Jul 20, 2009 at 07:44:06PM +0800, Da Zheng wrote:
> The fourth patch is the main one, which fully enable rpctrace to trace
> multitask programs.
>
> Zheng Da
>
> 2009-07-20 Zheng Da <zhengda1936@gmail.com>
>
> * rpctrace.c (UNKNOWN_NAME): New variable.
> (task_info): New structure.
> (traced_task): Removed.
> (task_ihash): New variable.
> (unknown_task): Likewise.
> (add_task): New function.
> (remove_task): Likewise.
> (traced_info): Modified.
> (receiver_info): New structure.
> (sender_info): Likewise.
> (send_once_info): Likewise.
> (TRACED_INFO): New macro.
> (SEND_INFO): Likewise.
> (SEND_ONCE_INFO): Likewise.
> (req_info): New structure.
> (req_head): New variable.
> (add_request): New function.
> (remove_request): Likewise.
> (freelist): Different type.
> (notify_pi): New variable.
> (receive_right_list): Likewise.
> (dummy_wrapper): Likewise.
> (traced_names): Different initial value.
> (other_class): New variable.
> (print_request_header): Different parameter.
> (print_reply_header): Likewise.
> (new_receiver_info): New function.
> (destroy_receiver_info): Likewise.
> (new_send_wrapper): Redefined.
> (new_send_once_wrapper): Modified.
> (unlink_sender_info): New function.
> (traced_dropweak): Removed.
> (traced_clean): New function.
> (seen_receive_right): Likewise.
> (discover_receive_right): Likewise.
> (get_send_wrapper): Likewise.
> (rewrite_right): Redefined.
> (print_contents): Don't treat mach_port_insert_right specially.
> (wrap_all_threads): Use new structuress.
> (wrap_new_thread): Likewise.
> (wrap_new_task): New function.
> (trace_and_forward): Redefined.
> (expected_reply_port): Removed.
> (print_request_header): Use new structures.
> (print_reply_header): Likewise.
> (unfinished_line): Removed.
> (traced_spawn): Use new structures.
> (main): Initialize some global variables.
Hm, this changelog looks a bit unspecific to my uneducated eye... You
probably won't get entry to the holy temply of perfect GNU patches with
this! ;-)
Rereading the changelog section in the GNU coding standards and/or some
of the older changlog entries for the Hurd might help I guess...
Do I care about nonperfect GNU-style changelogs? Not at all -- but
others might :-( [sigh]
> diff --git a/utils/rpctrace.c b/utils/rpctrace.c
> index 23e4e00..de7c9a4 100644
> --- a/utils/rpctrace.c
> +++ b/utils/rpctrace.c
> @@ -59,6 +59,8 @@ static const struct argp_option options[] =
> {0}
> };
>
> +#define UNKNOWN_NAME MACH_PORT_NULL
> +
> static const char args_doc[] = "COMMAND [ARG...]";
> static const char doc[] = "Trace Mach Remote Procedure Calls.";
>
> @@ -79,10 +81,43 @@ msgid_ihash_cleanup (void *element, void *arg)
> free (info);
> }
>
> +/* This structure stores the information of the traced task. */
> +struct task_info
> +{
> + task_t task;
> + boolean_t threads_wrapped; /* All threads of the task has been wrapped? */
> +};
> +
> static struct hurd_ihash msgid_ihash
> = HURD_IHASH_INITIALIZER (HURD_IHASH_NO_LOCP);
>
> -task_t traced_task;
> +static struct hurd_ihash task_ihash
> + = HURD_IHASH_INITIALIZER (HURD_IHASH_NO_LOCP);
> +
> +task_t unknown_task;
> +
> +void
> +add_task (task_t task)
> +{
> + error_t err;
> + struct task_info *info = malloc (sizeof *info);
> +
> + if (info == NULL)
> + error (1, 0, "Fail to allocate memory.");
Just handle it like most other functions:
error (1, errno, "malloc");
There is even precedence of this in the existing rpctrace code...
> +
> + info->task = task;
> + info->threads_wrapped = FALSE;
> +
Be careful with blanking: the blank line above is not really empty -- it
contains two space characters. Although this is not visible by default
in most editors (Vim can be configured to make it visible; I guess other
editors too), it's annoying to have such blank space errors in the code.
Certain editor commands won't work as expected...
And there are more lines like this in the patch.
(I was made aware of this by "git apply", which warns about this kind of
stuff.)
> + err = hurd_ihash_add (&task_ihash, task, info);
> + if (err)
> + error (1, err, "hurd_ihash_add");
> +}
> +
> +void
> +remove_task (task_t task)
> +{
> + hurd_ihash_remove (&task_ihash, 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
> @@ -188,45 +223,121 @@ msgid_trace_replies (const struct msgid_info *info)
> {
> return 1;
> }
> +
>
You still have a gratituous blank space change here... (Added a blank
line that wasn't there before.)
> -/* We keep one of these structures for each port right we are tracing. */
> +/* A common structure between sender_info and send_once_info */
> struct traced_info
> {
> struct port_info pi;
> -
> - mach_port_t forward; /* real port */
> mach_msg_type_name_t type;
> + char *name; /* null or a string describing this */
> +};
After reading the whole patch, I'm pretty convinced that there is really
no need for this common structure. There are one or two places in
trace_and_forward(), where sender_info and send_once_info are really
used interchangeably -- in *all* other places, we know at compile time
which one of the two we are actually dealing with.
So, how about just explicitely duplicating the fields in both
structures?... I think it will make the code much easier to follow.
(Perhaps we should discuss this on IRC.)
> +
> +/* Each traced port has one receiver info and multiple send wrappers.
> + * The receiver info records the information of the receive right to
> + * the traced port, while send wrappers are created for each task
> + * who has the send right to the traced port.
> + */
>
> +struct receiver_info
> +{
> char *name; /* null or a string describing this */
> + hurd_ihash_locp_t locp; /* position in the traced_names hash table */
> + mach_port_t portname; /* The port name in the owner task. */
> + task_t task; /* The task who has the right. */
> + mach_port_t forward; /* real port. */
> + struct receiver_info *receive_right; /* Link with other receive
> rights. */
> + struct sender_info *next; /* The head of the send right list */
The names of the last two fields are extremely confusing. "next" should
point to the next element in the list, which is the next receive right
-- i.e. what you named "receive_right". While the pointer to the head of
the send right list should be named "senders" or so.
> +};
> +
> +struct sender_info
> +{
> + struct traced_info pi;
> + task_t task; /* The task who has the right. */
>
> - union
> - {
> - struct traced_info *nextfree; /* Link when on free list. */
> -
> - struct /* For a send right wrapper. */
> - {
> - hurd_ihash_locp_t locp; /* position in the traced_names hash
> table */
> - } send;
> -
> - struct /* For a send-once right wrapper. */
> - {
> - /* We keep track of the send right to which the message containing
> - this send-once right as its reply port was sent, and the msgid of
> - that request. We don't hold a reference to the send right; it is
> - just a hint to indicate a match with a send right on which we just
> - forwarded a message. */
> - mach_port_t sent_to;
> - mach_msg_id_t sent_msgid;
> - } send_once;
> - } u;
> + /* It is used to form the list of send rights for different tasks.
> + * The head is the receive right. */
> + struct sender_info *next;
> +
> + struct receiver_info *receive_right; /* The corresponding receive
> right */
The name is confusing: this is not actually a port right, but rather a
pointer to the receiver info structure. It would be clearer to name it
just "receiver".
> };
> +
> +struct send_once_info
> +{
> + struct traced_info pi;
> + mach_port_t forward; /* real port. */
> +
> + struct send_once_info *nextfree; /* Link when on free list. */
I still think that the free list was a bad idea in the first place, and
it would be better to get rid of it alltogether... I vote for adding a
patch that drops the freelist stuff to the patch series, to go in before
this one.
> +};
> +
> #define INFO_SEND_ONCE(info) ((info)->type == MACH_MSG_TYPE_MOVE_SEND_ONCE)
> +#define TRACED_INFO(info) ((struct traced_info *) info)
> +#define SEND_INFO(info) ((struct sender_info *) info)
> +#define SEND_ONCE_INFO(info) ((struct send_once_info *) info)
Ugh, that's a rather ugly take at inheritence IMHO... I almost all
cases, you could just access the subfields directly, rather than doing
these casts.
But as I already said, I think it would be even better to make the
structs completely distinct, and don't use any such conversions at
all...
>
> -static struct traced_info *freelist;
> +/* This structure stores the information of the RPC requests. */
> +struct req_info
> +{
> + boolean_t is_req;
> + mach_msg_id_t req_id;
> + mach_port_t reply_port;
> + task_t from;
> + task_t to;
> + struct req_info *next;
> +};
I do not really understand how tracing of send-once rights works; so
please don't laugh too loudly if this question is stupid :-)
AIUI, in the original implementation, all information about an RPC
request was stored in the respective send-once structures; while you
have created an extra list for this. What is the reason for that
change?...
> +
> +static struct req_info *req_head = NULL;
> +
> +static struct req_info *
> +add_request (mach_msg_id_t req_id, mach_port_t reply_port,
> + task_t from, task_t to)
> +{
> + struct req_info *req = malloc (sizeof (*req));
> + if (!req)
> + error (1, 0, "cannot allocate memory");
> + req->req_id = req_id;
> + req->from = from;
> + req->to = to;
> + req->reply_port = reply_port;
> + req->is_req = TRUE;
> +
> + req->next = req_head;
> + req_head = req;
> +
> + return req;
> +}
> +
> +static struct req_info *
> +remove_request (mach_msg_id_t req_id, mach_port_t reply_port)
> +{
> + struct req_info **prev;
> + struct req_info *req;
> +
> + prev = &req_head;
> + while (*prev)
> + {
> + if ((*prev)->req_id == req_id && (*prev)->reply_port == reply_port)
> + break;
> + prev = &(*prev)->next;
> + }
Did I mention yet that I consider list traversal with for loops more
elegant?... Oh, I did? Never mind then -- just trying to push my private
agenda ;-)
> + if (*prev == NULL)
> + return NULL;
> +
> + req = *prev;
> + *prev = req->next;
> + return req;
> +}
> +
> +struct port_info *notify_pi;
> +/* The list of receiver infos, but only the ones for the traced tasks. */
> +struct receiver_info *receive_right_list;
Is the global receive right list really necessary? AIUI, we only ever
search it for receive rights in a specific task anyways -- so I think it
would be more efficient to have one list per task instead?
Moreover, I tend to think that it would make sense to use ihash
structures here as well...
> +static struct traced_info dummy_wrapper;
> +static struct send_once_info *freelist;
>
> struct hurd_ihash traced_names
> - = HURD_IHASH_INITIALIZER (offsetof (struct traced_info, u.send.locp));
> + = HURD_IHASH_INITIALIZER (offsetof (struct receiver_info, locp));
> struct port_class *traced_class;
> +struct port_class *other_class;
> struct port_bucket *traced_bucket;
> FILE *ostream;
>
> @@ -236,12 +347,13 @@ FILE *ostream;
> /* Called for a message that does not look like an RPC reply.
> The header has already been swapped into the sender's view
> with interposed ports. */
> -static void print_request_header (struct traced_info *info,
> +static void print_request_header (struct sender_info *info,
> mach_msg_header_t *header);
>
> /* Called for a message that looks like an RPC reply. */
> -static void print_reply_header (struct traced_info *info,
> - mig_reply_header_t *header);
> +static void print_reply_header (struct send_once_info *info,
> + mig_reply_header_t *header,
> + struct req_info *req);
>
> /* Called for each data item (which might be an array).
> Always called after one of the above two. */
> @@ -249,42 +361,106 @@ static void print_data (mach_msg_type_name_t type,
> const void *data,
> mach_msg_type_number_t nelt,
> mach_msg_type_number_t eltsize);
> +
>
Another gratituitous whitespace change...
> /*** Mechanics of tracing messages and interposing on ports ***/
>
> -
> -/* Create a new wrapper port and do `ports_get_right' on it. */
> -static struct traced_info *
> -new_send_wrapper (mach_port_t right, mach_port_t *wrapper_right)
> +/* Create a new info for the receive right.
> + * It lives until the traced receive right dies. */
I don't think this comment is completely correct? AIUI, the way things
are handled presently, the receiver info structure will also be
destroyed when all outstanding send rights get destroyed -- even if the
actual receive right still exists...
However, while reading other parts of the patch, I realized that perhaps
it might indeed be better to change this... I think it should be
possible to drop only the send right for the forward port when all
outstanding send right wrappers are gone, but still keep the actual
receiver info structure, until the port itself goes away. This could
save us some special handling in various places I believe -- more on
this below...
> +static struct receiver_info *
> +new_receiver_info (mach_port_t right, mach_port_t owner)
> {
> error_t err;
> - struct traced_info *info;
> -
> - /* Use a free send-once wrapper port if we have one. */
> - if (freelist)
> - {
> - info = freelist;
> - freelist = info->u.nextfree;
> - }
> - else
> - {
> - /* Create a new wrapper port that forwards to *RIGHT. */
> - err = ports_create_port (traced_class, traced_bucket,
> - sizeof *info, &info);
> - assert_perror (err);
> - info->name = 0;
> - }
> + struct receiver_info *info;
> + mach_port_t foo;
>
> + info = malloc (sizeof (*info));
> + if (!info)
> + error (1, 0, "cannot allocate memory");
> info->forward = right;
> - info->type = MACH_MSG_TYPE_MOVE_SEND;
> + info->task = owner;
> + info->portname = UNKNOWN_NAME;
> + info->receive_right = NULL;
> + info->next = NULL;
> + if (owner != unknown_task)
> + {
> + info->receive_right = receive_right_list;
> + receive_right_list = info;
> + }
> + info->name = 0;
> +
> + /* Request the dead-name notification, so if the receive right is
> destroyed,
> + * we can destroy the wrapper. */
> + MACH_NOTIFY_DEAD_NAME, 1,
> + notify_pi->port_right,
> + MACH_MSG_TYPE_MAKE_SEND_ONCE, &foo);
> + if (err)
> + error (2, err, "mach_port_request_notification");
> + mach_port_deallocate (mach_task_self (), foo);
>
> - /* Store it in the reverse-lookup hash table, so we can
> - look up this same right again to find the wrapper port.
> - The entry in the hash table holds a weak ref on INFO. */
> err = hurd_ihash_add (&traced_names, info->forward, info);
> + if (err)
> + error (2, err, "hurd_ihash_add");
> + return info;
> +}
> +
> +static void
> +destroy_receiver_info (struct receiver_info *info)
> +{
> + struct sender_info *send_wrapper;
> + struct receiver_info **prev;
> +
> + mach_port_deallocate (mach_task_self (), info->forward);
> + /* Remove it from the receive right list. */
> + prev = &receive_right_list;
> + while (*prev != info && *prev)
> + prev = &((*prev)->receive_right);
> + /* If we find the receiver info in the list. */
> + if (*prev)
> + *prev = info->receive_right;
> +
> + send_wrapper = info->next;
> + while (send_wrapper)
> + {
> + struct sender_info *next = send_wrapper->next;
> + assert (TRACED_INFO (send_wrapper)->pi.refcnt == 1);
> + /* Reset the receive_right of the send wrapper in advance to avoid
> + * destroy_receiver_info is called when the port info is destroyed. */
This is one of the cases where special handling can be avoided: if we
keep the receiver info structure even with no corresponding senders,
destroying a send wrapper will never need to invoke
destroy_receiver_info() -- instead, it can just drop the send right to
the forward port if it's the last sender for this port, but leave the
receiver info structure intact.
(This also means that destroy_receiver_info() won't need to drop the
right itself anymore. Dropping the forward right when the last wrapper
is dropped, and dropping the receiver info structure itself when the
actual receive right is dropped, are completely separate things this
way...)
> + send_wrapper->receive_right = NULL;
> + ports_destroy_right (send_wrapper);
> + send_wrapper = next;
> + }
> +
> + hurd_ihash_locp_remove (&traced_names, info->locp);
> + free (info);
> +}
> +
> +/* Create a new wrapper port and do `ports_get_right' on it.
> + *
> + * The wrapper lives until there is no send right to it,
> + * or the corresponding receiver info is destroyed.
> + */
> +static struct sender_info *
> +new_send_wrapper (struct receiver_info *receive, task_t task,
> + mach_port_t *wrapper_right)
> +{
> + error_t err;
> + struct sender_info *info;
> +
> + /* Create a new wrapper port that forwards to *RIGHT. */
You forgot to update this comment: there is no "*right" anymore here...
> + err = ports_create_port (traced_class, traced_bucket,
> + sizeof *info, &info);
> assert_perror (err);
> - ports_port_ref_weak (info);
> - assert (info->u.send.locp != 0);
> +
> + TRACED_INFO (info)->name = 0;
> + asprintf (&TRACED_INFO (info)->name, " %d<--%d(pid%d)",
> + receive->forward, TRACED_INFO (info)->pi.port_right, task2pid
> (task));
AIUI asprintf() always overwrites the previous contents of the pointer,
i.e. it's pointless to explicitely set it to 0 first...
Also, the result of asprintf() should always be checked for errors, just
like any other malloc() function.
> + TRACED_INFO (info)->type = MACH_MSG_TYPE_MOVE_SEND;
> + info->task = task;
> + info->receive_right = receive;
> + info->next = receive->next;
> + receive->next = info;
>
> *wrapper_right = ports_get_right (info);
> ports_port_deref (info);
Is it really right to drop the reference here, while "info" itself is
returned to the caller? Shouldn't we rather keep the reference, and let
the called drop it when it is done using it?...
(Sorry if this is a stupid question -- I'm still not very firm with this
reference counting stuff :-) )
> @@ -293,17 +469,17 @@ new_send_wrapper (mach_port_t right, mach_port_t
> *wrapper_right)
> }
>
> /* Create a new wrapper port and do `ports_get_right' on it. */
> -static struct traced_info *
> +static struct send_once_info *
> new_send_once_wrapper (mach_port_t right, mach_port_t *wrapper_right)
> {
> error_t err;
> - struct traced_info *info;
> + struct send_once_info *info;
>
> /* Use a free send-once wrapper port if we have one. */
> if (freelist)
> {
> info = freelist;
> - freelist = info->u.nextfree;
> + freelist = info->nextfree;
> }
> else
> {
> @@ -311,11 +487,12 @@ new_send_once_wrapper (mach_port_t right, mach_port_t
> *wrapper_right)
> err = ports_create_port (traced_class, traced_bucket,
> sizeof *info, &info);
> assert_perror (err);
> - info->name = 0;
> + TRACED_INFO (info)->name = 0;
> }
>
> info->forward = right;
> - info->type = MACH_MSG_TYPE_MOVE_SEND_ONCE;
> + TRACED_INFO (info)->type = MACH_MSG_TYPE_MOVE_SEND_ONCE;
> + info->nextfree = NULL;
>
> /* Send-once rights never compare equal to any other right (even
> another send-once right), so there is no point in putting them
> @@ -328,87 +505,286 @@ new_send_once_wrapper (mach_port_t right, mach_port_t
> *wrapper_right)
> receive a message on it. The kernel automatically sends a
> MACH_NOTIFY_SEND_ONCE message if the send-once right dies. */
>
> - *wrapper_right = info->pi.port_right;
> - memset (&info->u.send_once, 0, sizeof info->u.send_once);
> + *wrapper_right = TRACED_INFO (info)->pi.port_right;
>
> return info;
> }
>
> +/* Unlink the send wrapper from the list. */
> +static void
> +unlink_sender_info (void *pi)
> +{
> + struct sender_info *info = pi;
> + struct sender_info **prev;
> +
> + if (info->receive_right)
> + {
> + /* Remove it from the send right list. */
> + prev = &info->receive_right->next;
> + while (*prev != info && *prev)
> + prev = &((*prev)->next);
> + assert (*prev);
> + *prev = info->next;
> +
> + info->next = NULL;
> + }
> +}
>
> -/* This gets called when a wrapper port has no hard refs (send rights),
> - only weak refs. The only weak ref is the one held in the reverse-lookup
> - hash table. */
> +/* The function is called when the port_info is going to be destroyed.
> + * If it's the last send wrapper for the traced port, the receiver info
> + * will also be destroyed. */
Strange that I never noticed this before: you are using the wrong
comment style all the time... As you can clearly see in the above bit,
the existing rpctrace code (and other Hurd code) doesn't prefix the
comment continuation lines with '*'.
> + err = mach_port_request_notification (mach_task_self (), right,
> static void
> -traced_dropweak (void *pi)
> +traced_clean (void *pi)
> +{
> + struct sender_info *info = pi;
> +
> + assert (TRACED_INFO (info)->type == MACH_MSG_TYPE_MOVE_SEND);
> + free (TRACED_INFO (info)->name);
> +
> + if (info->receive_right)
> + {
> + unlink_sender_info (pi);
> +
> + /* If this is the last send wrapper, it means that our traced port
> won't
> + * have any more send rights. We notify the owner of the receive right
> + * of that by deallocating the forward port. */
> + if (info->receive_right->next == NULL)
> + destroy_receiver_info (info->receive_right);
> +
> + info->receive_right = NULL;
> + }
> +}
> +
> +/* Check if the receive right has been seen. */
> +boolean_t
> +seen_receive_right (task_t task, mach_port_t name)
> +{
> + struct receiver_info *info = receive_right_list;
> + while (info)
> + {
> + if (info->task == task && info->portname == name)
> + return TRUE;
> + info = info->receive_right;
> + }
> + return FALSE;
> +}
> +
> +/* This function is to find the receive right for the send right 'send'
> + * among traced tasks. I assume that all receive rights are moved
> + * under the control of rpctrace.
I think the comment should more explicitely state the purpose of the
function (get us a receiver info structure corresponding to the send
right), and how it goes about this (return an existing info structure if
we have one, or otherwise find the port in the target task and create a
new info structure for it).
> + *
> + * Note: 'send' shouldn't be the send right to the wrapper.
And what *should* it be?... :-)
You could state more explicitely that this function deals with a "real"
send right provided by the task holding the corresponding receive right.
This way it should be clear that it is not about dealing with wrapper
ports -- the explicit note might not even be necessary...
> + *
> + * Note: the receiver_info returned from the function
> + * might not be the receive right in the traced tasks.
I think you mean that some receive rights are held by "unknown"
tasks?...
(Some of your sentences are hard to understand, because you often use a
definite article ("the"), where an indefinite article ("a"/"an") should
be, or a quantifier ("some"/"any"), or no article at all. I believe that
learning the proper use of articles would be the one thing necessary for
you to greatly improve your English skills :-) )
I think it would be clearer and more informative to explain that we
check under what port name the right resides in the task, unless the
receive right is held by some external task (not under the control of
rpctrace).
> + * */
> +struct receiver_info *
> +discover_receive_right (mach_port_t send, task_t task)
> {
> - struct traced_info *const info = pi;
> + error_t err;
> + struct receiver_info *info = NULL;
No need to initialize it, as the first thing you do is setting this
anyways...
>
> - assert (info->type == MACH_MSG_TYPE_MOVE_SEND);
> - assert (info->u.send.locp);
> + info = hurd_ihash_find (&traced_names, send);
> + /* If we have seen the send right or send once right. */
> + if (info
> + /* If the receive right is in one of traced tasks,
> + * but we don't know its name
> + * (probably because the receive right has been moved),
> + * we need to find it out. */
> + && !(info->task != unknown_task
> + && info->portname == UNKNOWN_NAME))
> + return info;
Seems to me that both the condition and the comment would become
clearer, if instead of returning early when all info is present, the
condition was inverted, and the whole search loop put in the conditional
block...
> +
Another non-emply blank line.
> + {
I think it would be useful to have a comment for the whole code block,
explaining how the search is performed. (Obtain all ports in the task;
for each receive right that has any send rights, get a send right; if
it's the same as the one we are looking for, we have found the desired
receive right, and create an info structure for it that we can return.)
> + int j;
> + mach_port_t *portnames = NULL;
> + mach_msg_type_number_t nportnames = 0;
> + mach_port_type_t *porttypes = NULL;
> + mach_msg_type_number_t nporttypes = 0;
> + struct receiver_info *receiver_info = NULL;
I don't think we need an extra variable: it has the very same function
as "info" outside this block -- so it would be more logical to reuse the
same variable here.
>
> - /* Remove INFO from the hash table. */
> - hurd_ihash_locp_remove (&traced_names, info->u.send.locp);
> - ports_port_deref_weak (info);
> + err = mach_port_names (task, &portnames, &nportnames,
> + &porttypes, &nporttypes);
> + if (err == MACH_SEND_INVALID_DEST)
> + {
> + remove_task (task);
> + return 0;
> + }
Didn't we agree that if a traced task unexpectedly disappears under our
feet, something must have gone seriously wrong, and we better make a lot
of noise and bail out immediately, rather than silently ignoring it?...
> + if (err)
> + error (2, err, "mach_port_names");
>
> - /* Deallocate the forward port, so the real port also sees no-senders. */
> - mach_port_deallocate (mach_task_self (), info->forward);
> + for (j = 0; j < nportnames; j++)
> + {
> + mach_port_status_t port_status;
> + mach_port_t send_right;
> + mach_msg_type_name_t type;
>
> - /* There are no rights to this port, so we can reuse it.
> - Add a hard ref and put INFO on the free list. */
> - ports_port_ref (info);
> + if (!(porttypes[j] & MACH_PORT_TYPE_RECEIVE) /* not a receive right */
> + || seen_receive_right (task, portnames[j]))
> + continue;
>
> - free (info->name);
> - info->name = 0;
> + err = mach_port_get_receive_status (task, portnames[j],
> + &port_status);
> + if (err)
> + error (2, err, "mach_port_get_receive_status");
> + /* If the port doesn't have the send right, skip it. */
"If there are no send rights for this port, skip it."
> + if (!port_status.mps_srights)
> + continue;
>
> - info->u.nextfree = freelist;
> - freelist = info;
> + err = mach_port_extract_right (task, portnames[j],
> + MACH_MSG_TYPE_MAKE_SEND,
> + &send_right, &type);
> + if (err)
> + error (2, err, "mach_port_extract_right");
> +
> + if (/* We have seen this send right before. */
> + hurd_ihash_find (&traced_names, send_right)
> + || send_right != send /* It's not the port we want. */)
It's a bit confusing that you put the comment for the first part of the
condition before the bit it refers to, and the comment for the second
part after the relevant bit... Please be consistent here :-)
> + {
> + mach_port_deallocate (mach_task_self (), send_right);
> + continue;
> + }
> +
> + /* We have found the receive right we want. */
> + receiver_info = new_receiver_info (send_right, task);
> + receiver_info->portname = portnames[j];
> + break;
> + }
> + if (portnames)
> + vm_deallocate (mach_task_self (), (vm_address_t) portnames,
> + nportnames * sizeof (*portnames));
> + if (porttypes)
> + vm_deallocate (mach_task_self (), (vm_address_t) porttypes,
> + nporttypes * sizeof (*porttypes));
> +
> + if (receiver_info)
> + return receiver_info;
> + }
> + return NULL;
We don't need the last if, nor the extra NULL return -- always returning
receiver_info should have exactly the same effect.
> }
>
> +/* get_send_wrapper searches for the send wrapper for the target task.
> + If it doesn't exist, create a new one. */
> +struct sender_info *
> +get_send_wrapper (struct receiver_info *receiver_info,
> + mach_port_t task, mach_port_t *right)
> +{
> + struct sender_info *info = receiver_info->next;
> +
> + while (info)
> + {
> + if (info->task == task)
> + {
> + *right = ports_get_right (info);
> + return info;
> + }
> + info = info->next;
> + }
> + /* No send wrapper is found. */
> + return new_send_wrapper (receiver_info, task, right);
> +}
>
> /* Rewrite a port right in a message with an appropriate wrapper port. */
> -static struct traced_info *
> -rewrite_right (mach_port_t *right, mach_msg_type_name_t *type)
> +static char *
> +rewrite_right (mach_port_t *right, mach_msg_type_name_t *type,
> + struct req_info *req)
> {
I think the comment should also explain the return value...
> error_t err;
> - struct traced_info *info;
> + struct receiver_info *receiver_info;
> + struct sender_info *send_wrapper;
> + task_t dest = unknown_task;
> + task_t source = unknown_task;
>
> /* We can never do anything special with a null or dead port right. */
> if (!MACH_PORT_VALID (*right))
> return 0;
>
> + if (req)
> + {
> + if (req->is_req) /* It's a RPC request. */
> + {
> + source = req->from;
> + dest = req->to;
> + }
> + else
> + {
> + source = req->to;
> + dest = req->from;
> + }
> + }
The naming is rather confusing here -- if it can be both a request or a
reply, I suggest naming it something else than "req"...
More importantly, I have some serious doubts about the use of
source/dest. Why does it matter who initiated the RPC? Isn't it more
relevant who the sender/receiver of the message is, no matter whether
it's an RPC request or an RPC reply?
Am I missing something crucial here?...
> +
> switch (*type)
> {
> case MACH_MSG_TYPE_PORT_SEND:
> - /* See if we are already tracing this port. */
> - info = hurd_ihash_find (&traced_names, *right);
> - if (info)
> - {
> - /* We are already tracing this port. We will pass on a right
> - to our existing wrapper port. */
> - *right = ports_get_right (info);
> - *type = MACH_MSG_TYPE_MAKE_SEND;
> - return info;
> - }
> + /* The strategy for moving the send right is: if the destination task
> + * has the receive right, we move the send right of the traced port to
> + * the destination; otherwise, we move the one of the send wrapper.
> + */
Well, this comment describes only the generic case (making the comment
somewhat below mostly redundant), but doesn't say anything about the
special handling of unknown tasks...
However, while thinking how this could be improved, I realized that the
whole code block would probably become clearer with some refactoring:
make the distinction explained in the above comment the outer
conditional, and handle the special case of the sender being an unknown
task inside this. So it would look something like this:
if (send_wrapper)
receiver_info = send_wrapper->receive_right;
else
receiver_info = discover_receive_right();
if (dest == receiver_info->task)
if (send_wrapper)
copy forward send right
else
move bare send right (no rewriting)
else
create new wrapper
(This leaves out some special case handling etc. of course, but I hope
you get the idea.)
What do you think, is this a good idea? Perhaps we should discuss the
details on IRC...
> + assert (req);
>
> /* See if this is already one of our own wrapper ports. */
> - info = ports_lookup_port (traced_bucket, *right, 0);
> - if (info)
> + send_wrapper = ports_lookup_port (traced_bucket, *right, 0);
> + if (send_wrapper)
> {
> - /* This is a send right to one of our own wrapper ports.
> - Instead, send along the original send right. */
> + /* This is a send right to one of our own wrapper ports. */
> mach_port_deallocate (mach_task_self (), *right); /* eat msg ref */
> - *right = info->forward;
> - err = mach_port_mod_refs (mach_task_self (), *right,
> - MACH_PORT_RIGHT_SEND, +1);
> - assert_perror (err);
> - ports_port_deref (info);
> - return info;
> +
> + /* If the send right is moved to the task with the receive right,
> + * copy the send right in 'forward' of receiver info to the
> destination.
> + * Otherwise, copy the send right to the send wrapper. */
> + assert (send_wrapper->receive_right);
> + if (dest == send_wrapper->receive_right->task)
> + {
> + *right = send_wrapper->receive_right->forward;
> + err = mach_port_mod_refs (mach_task_self (), *right,
> + MACH_PORT_RIGHT_SEND, +1);
> + if (err)
> + error (2, err, "mach_port_mod_refs");
> + ports_port_deref (send_wrapper);
> + }
> + else
> + {
> + struct sender_info *send_wrapper2
I'd prefer a more meaningful name, e.g. "dest_send_wrapper".
> + = get_send_wrapper (send_wrapper->receive_right, dest, right);
> + ports_port_deref (send_wrapper);
> + *type = MACH_MSG_TYPE_MAKE_SEND;
> + send_wrapper = send_wrapper2;
> + }
> + return TRACED_INFO (send_wrapper)->name;
> }
>
> - /* We have never seen this port before. Create a new wrapper port
> - and replace the right in the message with a right to it. */
> - *type = MACH_MSG_TYPE_MAKE_SEND;
> - return new_send_wrapper (*right, right);
> + if (req->req_id == 3216) /* mach_port_extract_right */
> + receiver_info = discover_receive_right (*right, dest);
> + else
> + receiver_info = discover_receive_right (*right, source);
It seems to me that the only reason why you need special handling here,
is because mach_port_extract_right() transfers a port right in the reply
message, rather than the request message. However, it is by no means the
only RPC that does this. Wouldn't it be more correct to just always
check the sender (i.e. req->from) ?...
(See my remark aber source/dest above.)
> + if (receiver_info == NULL)
> + {
> + /* It's unusual to see an unknown send right from a traced task.
> + * We ignore it. */
> + if (source != unknown_task)
> + {
> + error (0, 0, "get an unknown send right from process %d",
> + task2pid (source));
> + return dummy_wrapper.name;
> + }
You explained in what situation this can happen in your mail reply to my
question regarding this; but the actual comment still doesn't contain
any explanation... People should be able to understand the code without
having to know about an explanation that exists only in a certain
mailing list message :-)
> + /* The receive right is owned by an unknown task. */
> + receiver_info = new_receiver_info (*right, unknown_task);
I think it would be more appropriate to do this in
discover_receive_right(): after all, that function does return an
existing receiver info structure, or create one for traced tasks -- why
not also create one for unknown tasks? Seems rather inconsistent to
leave it to the caller in this specific case only...
> + mach_port_mod_refs (mach_task_self (), *right,
> + MACH_PORT_RIGHT_SEND, 1);
> + }
> + /* If the send right is moved to the task with the receive right,
> + * don't do anything.
> + * Otherwise, we translate it into the one to the send wrapper. */
> + if (dest == receiver_info->task)
> + return receiver_info->name;
> + else
> + {
> + assert (*right == receiver_info->forward);
> + mach_port_deallocate (mach_task_self (), *right);
> + send_wrapper = get_send_wrapper (receiver_info, dest, right);
> + *type = MACH_MSG_TYPE_MAKE_SEND;
> + return TRACED_INFO (send_wrapper)->name;
> + }
>
> case MACH_MSG_TYPE_PORT_SEND_ONCE:
> /* There is no way to know if this send-once right is to the same
> @@ -418,62 +794,99 @@ rewrite_right (mach_port_t *right, mach_msg_type_name_t
> *type)
> make a new send-once wrapper object, that will trace the one
> message it receives, and then die. */
> *type = MACH_MSG_TYPE_MAKE_SEND_ONCE;
> - return new_send_once_wrapper (*right, right);
> + return TRACED_INFO (new_send_once_wrapper (*right, right))->name;
>
> case MACH_MSG_TYPE_PORT_RECEIVE:
> - /* We have got a receive right, call it A. We will pass along a
> - different receive right of our own, call it B. We ourselves will
> - receive messages on A, trace them, and forward them on to B.
> -
> - If A is the receive right to a send right that we have wrapped,
> - then B must be that wrapper receive right, moved from us to the
> - intended receiver of A--that way it matches previous send rights
> - to A that were sent through and replaced with our wrapper (B).
> - If not, we create a new receive right. */
> + /* We have got a receive right, call it A and the send wrapper for
> + * the destination task is denoted as B (if the destination task
> + * doesn't have the send wrapper, we create it before moving receive
> + * right).
> + * We wrap the receive right A in the send wrapper and move the receive
> + * right B to the destination task. */
I fear this comment is very hard to understand -- even *knowing* what it
is trying to say, I have a hard time understanding it...
I'd try to explain it like this:
In the most generic case, both the origin task and the destination
task have send rights corresponding to the receive right being moved.
The one in the origin task is a "real" send right for the port --
it's the same task, so there is no wrapping on this end. (And thus
also no sender_info structure.) It's the same send right as the
"forward" send right stored in the receiver_info structure.
The receive right in the destination task on the other hand is a
wrapper port right, with a sender_info structure connected to the
receiver_info structure of the receive right in question.
When the receive right is transferred, the roles of the tasks are
reversed, and the wrapping must be reversed too: what used to be the
destination task's wrapper port so far, will be considered the real
port now; and the port in the origin task will turn into a wrapper
port. The receiver_info structure will point to the destination task
now, and the "forward" send right for the origin task's port stored
in it up till now will be replaced by the send right for the
destination task's (former) wrapper port.
Note that the receive right which the destination task obtains, is
actually not the one the origin task passed. Rather, it is the
receive right for what used to be the destination task's wrapper
port. It is taken from the destination task's sender_info structure;
while this sender_info structure itself is dropped now.
The actual receive right passed from the origin task on the other
hand is stored in a new sender_info for the origin task -- the port
turns into a wrapper port now.
If the destination task didn't actually hold any (wrapper) receive
right for the port, and thus has no sender_info structure, a
(temporary) structure is automatically created by get_send_wrapper().
If the origin task didn't actually have any send right, a "no
senders" notification is generated as soon as the "forward" send
right from the receiver_info structure is dropped (after replacing it
with the destination task's one), and the newly created sender_info
for the origin task is dropped again.
(BTW, the origin and/or destination task not actually having a receive
right, is actually the most common situation I believe -- so it could
make quite a differece if we special-case this, instead of creating
send_wrapper structures only to drop them again immediately... This
would only be an optimisation though; and thus should go into a pair of
followup patches rather than the main one I guess.)
I think that it would help understanding also to reorder the actual code
a bit to more closely follow the explanation -- except that the order of
actions is a bit different of course: first create the new sender_info
for the origin task, using the receive right passed in the message as
the wrapper receive right, and link it with the receiver_info; then drop
the original forward send right (which will generate "no senders" if the
origin task had no send right); then extract the send right from the
destination task's sender_info, storing it as the new forward port, and
drop the destination task's sender_info.
> {
> - mach_port_t rr; /* B */
> - char *name;
> -
> - info = hurd_ihash_find (&traced_names, *right);
> - if (info)
> + assert (req);
> + receiver_info = hurd_ihash_find (&traced_names, *right);
> + if (receiver_info)
> {
> - /* This is a receive right that we have been tracing sends to. */
> - name = info->name;
> - rr = ports_claim_right (info);
> - /* That released the refs on INFO, so it's been freed now. */
> + struct sender_info *send_wrapper2;
Again, something like "dest_send_wrapper" would be more informative.
> + char *name;
> + mach_port_t rr;
You could use a more descriptive name, or at least add a comment... It's
not at all clear that this is the port name of the right passed to the
destination task, as opposed to "right", which is the one from the
source task.
> +
> + /* The port A has at least one send right - the one in
> + * receiver_info->forward. If the source task doesn't have
> + * the send right, the port A will be destroyed after we
> + * deallocate the only send right. */
> +
> + /* We have to deallocate the send right in
> + * receiver_info->forward before we import the port to port_info.
> + * So the reference count in the imported port info will be 1,
> + * if it doesn't have any other send rights. */
I don't understand how I still failed to be clear on this: this is *not*
the right explanation! :-(
We drop the forward send right, because we won't be forwarding to this
port anymore. (The roles are reversed: from now on, the port in the
destination task will be considered the "real" port we forward to.) This
is what the comment should say -- nothing less and nothing more.
Also this would become much clearer, if you move this statement to the
place where receiver_info->forward actually gets the new value
assigned... It doesn't realy have much to do with the creation of the
new sender_info for the source task.
> + mach_port_deallocate (mach_task_self (), receiver_info->forward);
> + err = ports_import_port (traced_class, traced_bucket,
> + *right, sizeof *send_wrapper,
> + &send_wrapper);
> + if (err)
> + error (2, err, "ports_import_port");
I think it would be useful to state explicitely that a send wrapper for
the source task is created here -- this is clear to someone who
understands the code of course, but I'm not sure it's obvious when
reading it for the first time...
> +
> + TRACED_INFO (send_wrapper)->type = MACH_MSG_TYPE_MOVE_SEND;
> + send_wrapper->task = source;
> + TRACED_INFO (send_wrapper)->name = receiver_info->name;
> + /* Initialize them in case that the source task doesn't
> + * have the send right to the port, and the port will
> + * be destroyed immediately. */
This bit is above me: I don't see how the port can be destroyed between
here, and the code that sets the "proper" values below...
Also, the assignments below are unconditional -- so the port *must*
still exists there?...
To be honest, it's not entirely clear to me where exactly the port gets
destroyed.
> + send_wrapper->receive_right = NULL;
> + send_wrapper->next = NULL;
> + ports_port_deref (send_wrapper);
> +
> + hurd_ihash_locp_remove (&traced_names, receiver_info->locp);
> +
> + send_wrapper2 = get_send_wrapper (receiver_info, dest, &rr);
> + assert (TRACED_INFO (send_wrapper2)->pi.refcnt == 1);
> + name = TRACED_INFO (send_wrapper2)->name;
> + TRACED_INFO (send_wrapper2)->name = NULL;
> + /* send_wrapper2 isn't destroyed normally, so we need to unlink
> + * it from the send wrapper list before calling ports_claim_right */
I think this could do with some further explanation too: in what sense
it is not destroyed "normally"?...
> + unlink_sender_info (send_wrapper2);
> + send_wrapper2->receive_right = NULL;
> + rr = ports_claim_right (send_wrapper2);
> + /* Get us a send right that we will forward on. */
> + err = mach_port_insert_right (mach_task_self (), rr, rr,
> + MACH_MSG_TYPE_MAKE_SEND);
> + if (err)
> + error (2, err, "mach_port_insert_right");
> + receiver_info->forward = rr;
> + receiver_info->task = dest;
> + if (dest != unknown_task)
> + {
> + receiver_info->receive_right = receive_right_list;
> + receive_right_list = receiver_info;
> + }
> + /* The port name will be discovered
> + * when we search for this receive right. */
> + receiver_info->portname = UNKNOWN_NAME;
Searching for the port right is a very expensive operation; so I think
it would make quite a difference to special-case those situations where
we already know the port name... (i.e. mach_port_insert_right() )
Of course this is also an optimization that probably should go in an
extra followup patch.
In either case, I think the possibility should be mentioned in the
comment at least...
> + receiver_info->name = name;
> +
> + send_wrapper->receive_right = receiver_info;
> + send_wrapper->next = receiver_info->next;
> + receiver_info->next = send_wrapper;
> +
> + err = hurd_ihash_add (&traced_names, receiver_info->forward,
> + receiver_info);
> + if (err)
> + error (2, err, "hurd_ihash_add");
> + *right = rr;
> }
> else
> {
> - /* This is a port we know nothing about. */
> - rr = mach_reply_port ();
> - name = 0;
> + /* Weird? no send right for the port. */
IIRC this is another situation that you explained in your mail reply,
but haven't updated the comment to explain it too...
> + err = mach_port_insert_right (mach_task_self (), *right, *right,
> + MACH_MSG_TYPE_MAKE_SEND);
> + if (err)
> + error (2, err, "mach_port_insert_right");
> + receiver_info = new_receiver_info (*right, dest);
I believe this is another case that we could handle more elegantly, if
we allow receiver_info structures to exist without a send right. In
fact, creating a new send right here really seems wrong -- I tend to
think that it can cause actual misbehaviour when this code is
triggered...
> }
>
> - /* Create a new wrapper object that receives on this port. */
> - err = ports_import_port (traced_class, traced_bucket,
> - *right, sizeof *info, &info);
> - assert_perror (err);
> - info->name = name;
> - info->type = MACH_MSG_TYPE_MOVE_SEND; /* XXX ? */
> -
> - /* Get us a send right that we will forward on. */
> - err = mach_port_insert_right (mach_task_self (), rr, rr,
> - MACH_MSG_TYPE_MAKE_SEND);
> - assert_perror (err);
> - info->forward = rr;
> -
> - err = hurd_ihash_add (&traced_names, info->forward, info);
> - assert_perror (err);
> - ports_port_ref_weak (info);
> -
> - /* If there are no extant send rights to this port, then INFO will
> - die right here and release its send right to RR.
> - XXX what to do?
> - */
> - ports_port_deref (info);
> -
> - *right = rr;
> - return info;
> + return receiver_info->name;
> }
>
> default:
> @@ -484,7 +897,7 @@ rewrite_right (mach_port_t *right, mach_msg_type_name_t
> *type)
>
> static void
> print_contents (mach_msg_header_t *inp,
> - void *msg_buf_ptr)
> + void *msg_buf_ptr, struct req_info *req)
> {
> error_t err;
>
> @@ -543,7 +956,6 @@ print_contents (mach_msg_header_t *inp,
> mach_msg_type_number_t i;
> mach_msg_type_name_t newtypes[nelt];
> int poly;
> - struct traced_info *ti;
>
> assert (inp->msgh_bits & MACH_MSGH_BITS_COMPLEX);
> assert (eltsize == sizeof (mach_port_t));
> @@ -551,19 +963,11 @@ print_contents (mach_msg_header_t *inp,
> poly = 0;
> for (i = 0; i < nelt; ++i)
> {
> + char *str;
> +
> newtypes[i] = name;
>
> - if (inp->msgh_id == 3215) /* mach_port_insert_right */
> - {
> - /* XXX
> - */
> - fprintf (ostream,
> - "\t\t[%d] = pass through port %d, type %d\n",
> - i, portnames[i], name);
> - continue;
> - }
> -
> - ti = rewrite_right (&portnames[i], &newtypes[i]);
> + str = rewrite_right (&portnames[i], &newtypes[i], req);
>
> putc ((i == 0 && nelt > 1) ? '{' : ' ', ostream);
>
> @@ -573,9 +977,8 @@ print_contents (mach_msg_header_t *inp,
> fprintf (ostream, "(dead)");
> else
> {
> - assert (ti);
> - if (ti->name != 0)
> - fprintf (ostream, "%s", ti->name);
> + if (str != 0)
> + fprintf (ostream, "%s", str);
> else
> fprintf (ostream, "%3u", (unsigned int) portnames[i]);
> }
> @@ -646,11 +1049,12 @@ print_contents (mach_msg_header_t *inp,
> }
> }
>
> -/* Wrap all thread port in the task */
> +/* Wrap all thread ports in the task */
Please fix this in the patch that actually introduced this comment,
rather than here :-)
> static void
> wrap_all_threads (task_t task)
> {
> - struct traced_info *thread_send_wrapper;
> + struct sender_info *thread_send_wrapper;
> + struct receiver_info *thread_receiver_info;
> thread_t *threads = NULL;
> size_t nthreads = 0;
> error_t err;
> @@ -661,14 +1065,18 @@ wrap_all_threads (task_t task)
>
> for (int i = 0; i < nthreads; i++)
> {
> - thread_send_wrapper = hurd_ihash_find (&traced_names, threads[i]);
> + thread_receiver_info = hurd_ihash_find (&traced_names, threads[i]);
> /* We haven't seen the port. */
> - if (thread_send_wrapper == NULL)
> + if (thread_receiver_info == 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]);
> +
> + thread_receiver_info = new_receiver_info (threads[i], unknown_task);
> + thread_send_wrapper = new_send_wrapper (thread_receiver_info,
> + task, &new_thread_port);
> + free (TRACED_INFO (thread_send_wrapper)->name);
> + asprintf (&TRACED_INFO (thread_send_wrapper)->name,
> + "thread%d(pid%d)", threads[i], task2pid (task));
>
> err = mach_port_insert_right (mach_task_self (),
> new_thread_port, new_thread_port,
> @@ -688,7 +1096,7 @@ wrap_all_threads (task_t task)
>
> /* Wrap the new thread port that is in the message. */
> static void
> -wrap_new_thread (mach_msg_header_t *inp)
> +wrap_new_thread (mach_msg_header_t *inp, struct req_info *req)
> {
> error_t err;
> mach_port_t thread_port;
> @@ -702,27 +1110,81 @@ wrap_new_thread (mach_msg_header_t *inp)
> } *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,
> + struct sender_info *send_wrapper = ports_lookup_port (traced_bucket,
> reply->child_thread, 0);
>
> assert (send_wrapper);
> + assert (send_wrapper->receive_right);
> 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;
> + thread_port = send_wrapper->receive_right->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);
> + free (TRACED_INFO (send_wrapper)->name);
> + asprintf (&TRACED_INFO (send_wrapper)->name, "thread%d(pid%d)",
> + thread_port, task2pid (req->from));
> mach_port_deallocate (mach_task_self (), reply->child_thread);
> ports_port_deref (send_wrapper);
> }
>
> +/* Wrap the new task port that is in the message. */
> +static void
> +wrap_new_task (mach_msg_header_t *inp, struct req_info *req)
> +{
> + error_t err;
> + pid_t pid;
> + task_t pseudo_task_port;
> + task_t task_port;
> + struct
> + {
> + mach_msg_header_t head;
> + mach_msg_type_t retcode_type;
> + kern_return_t retcode;
> + mach_msg_type_t child_task_type;
> + mach_port_t child_task;
> + } *reply = (void *) inp;
> + /* The send wrapper of the new task for the father task */
> + struct sender_info *task_wrapper1 = ports_lookup_port (traced_bucket,
> + reply->child_task, 0);
> + /* The send wrapper for the new task itself. */
> + struct sender_info *task_wrapper2;
Again, I vote for more expressive names :-) e.g. "parent_task_wrapper"
and "own_task_wrapper" or so...
I guess it would be also good to mention somewhere that we need the
pseudo task port in order to intercept port manipulation RPCs done by
the task itself. And that the wrapper for the parent task has already
been crated by the usual port wrapping mechanism.
> +
> + assert (task_wrapper1);
> + assert (task_wrapper1->receive_right);
> +
> + task_port = task_wrapper1->receive_right->forward;
> + add_task (task_port);
> +
> + task_wrapper2 = new_send_wrapper (task_wrapper1->receive_right,
> + task_port, &pseudo_task_port);
> + err = mach_port_insert_right (mach_task_self (),
> + pseudo_task_port, pseudo_task_port,
> + MACH_MSG_TYPE_MAKE_SEND);
> + if (err)
> + error (2, err, "mach_port_insert_right");
> + err = task_set_kernel_port (task_port, pseudo_task_port);
> + if (err)
> + error (2, err, "task_set_kernel_port");
> + mach_port_deallocate (mach_task_self (), pseudo_task_port);
> +
> + pid = task2pid (task_port);
> + free (TRACED_INFO (task_wrapper1)->name);
> + asprintf (&TRACED_INFO (task_wrapper1)->name, "task%d(pid%d)",
> + task_port, task2pid (req->from));
> + free (TRACED_INFO (task_wrapper2)->name);
> + asprintf (&TRACED_INFO (task_wrapper2)->name, "task%d(pid%d)",
> + task_port, pid);
> + ports_port_deref (task_wrapper1);
> +}
> +
> int
> trace_and_forward (mach_msg_header_t *inp, mach_msg_header_t *outp)
> {
> + mach_port_t reply_port;
> +
> const mach_msg_type_t RetCodeType =
> {
> MACH_MSG_TYPE_INTEGER_32, /* msgt_name = */
> @@ -749,19 +1211,28 @@ trace_and_forward (mach_msg_header_t *inp,
> mach_msg_header_t *outp)
This is not visible in the patch, but it seems that the comment for the
ports_lookup_port() call isn't correct anymore... (It says the class
needn't be checked, as there is only traced_class -- but you added
other_class...)
> with a send-once right, even if there have never really been any. */
> if (MACH_MSGH_BITS_LOCAL (inp->msgh_bits) == MACH_MSG_TYPE_MOVE_SEND_ONCE)
> {
> - if (inp->msgh_id == MACH_NOTIFY_DEAD_NAME)
> + if (inp->msgh_id == MACH_NOTIFY_DEAD_NAME && info == (void *)
> notify_pi)
> {
> - /* If INFO is a send-once wrapper, this could be a forged
> - notification; oh well. XXX */
> -
> + struct receiver_info *receiver_info;
> const mach_dead_name_notification_t *const n = (void *) inp;
>
> - assert (n->not_port == info->forward);
> /* Deallocate extra ref allocated by the notification. */
> mach_port_deallocate (mach_task_self (), n->not_port);
> - ports_destroy_right (info);
> - ports_port_deref (info);
> + receiver_info = hurd_ihash_find (&traced_names, n->not_port);
> + /* The receiver info might have been destroyed.
> + * If not, we destroy it here. */
Again, you explained how this can happen in your mail reply, but didn't
add the explanation to the actual comment...
> + if (receiver_info)
> + {
> + assert (n->not_port == receiver_info->forward);
> + destroy_receiver_info (receiver_info);
> + }
> +
> ((mig_reply_header_t *) outp)->RetCode = MIG_NO_REPLY;
> + ports_port_deref (info);
> +
> + /* It might be a task port. Remove the dead task from the list. */
> + remove_task (n->not_port);
> +
> return 1;
> }
> else if (inp->msgh_id == MACH_NOTIFY_NO_SENDERS
> @@ -776,8 +1247,17 @@ trace_and_forward (mach_msg_header_t *inp,
> mach_msg_header_t *outp)
> ((mig_reply_header_t *) outp)->RetCode = MIG_NO_REPLY;
> return 1;
> }
> + /* Get some unexpected notification for rpctrace itself,
> + * TODO ignore them for now. */
> + else if (info == (void *) notify_pi)
> + {
> + ports_port_deref (info);
> + ((mig_reply_header_t *) outp)->RetCode = MIG_NO_REPLY;
> + return 1;
> + }
> }
>
> + assert (info != (void *) notify_pi);
> assert (MACH_MSGH_BITS_LOCAL (inp->msgh_bits) == info->type);
>
> complex = inp->msgh_bits & MACH_MSGH_BITS_COMPLEX;
> @@ -788,31 +1268,43 @@ trace_and_forward (mach_msg_header_t *inp,
> mach_msg_header_t *outp)
> {
> mach_msg_type_name_t this_type = MACH_MSGH_BITS_LOCAL (inp->msgh_bits);
> mach_msg_type_name_t reply_type = MACH_MSGH_BITS_REMOTE (inp->msgh_bits);
> +
> + /* Save the original reply port in the RPC request. */
> + reply_port = inp->msgh_remote_port;
>
> inp->msgh_local_port = inp->msgh_remote_port;
> - if (reply_type && msgid_trace_replies (msgid))
> + if (reply_type && msgid_trace_replies (msgid)
> + /* The reply port might be dead, e.g., the traced task has died. */
> + && MACH_PORT_VALID (inp->msgh_local_port))
This doesn't really look like it is related to tracing multiple
tasks?... (I would assume it is a general bug fix that should go in a
separate patch?)
> {
> - struct traced_info *info;
> - info = rewrite_right (&inp->msgh_local_port, &reply_type);
> - assert (info);
> - if (info->name == 0)
> + struct send_once_info *info;
> + // TODO is the reply port always a send once right?
> + assert (reply_type == MACH_MSG_TYPE_PORT_SEND_ONCE);
> + info = new_send_once_wrapper (inp->msgh_local_port,
> + &inp->msgh_local_port);
> + reply_type = MACH_MSG_TYPE_MAKE_SEND_ONCE;
Why have you replicated the code here, instead of using rewrite_right()
as before?...
> + assert (inp->msgh_local_port);
> +
> + if (TRACED_INFO (info)->name == 0)
> {
> if (msgid == 0)
> - asprintf (&info->name, "reply(%u:%u)",
> - (unsigned int) info->pi.port_right,
> + asprintf (&TRACED_INFO (info)->name, "reply(%u:%u)",
> + (unsigned int) TRACED_INFO (info)->pi.port_right,
> (unsigned int) inp->msgh_id);
> else
> - asprintf (&info->name, "reply(%u:%s)",
> - (unsigned int) info->pi.port_right, msgid->name);
> - }
> - if (info->type == MACH_MSG_TYPE_MOVE_SEND_ONCE)
> - {
> - info->u.send_once.sent_to = info->pi.port_right;
> - info->u.send_once.sent_msgid = inp->msgh_id;
> + asprintf (&TRACED_INFO (info)->name, "reply(%u:%s)",
> + (unsigned int) TRACED_INFO (info)->pi.port_right,
> + msgid->name);
> }
> }
>
> - inp->msgh_remote_port = info->forward;
> + if (info->type == MACH_MSG_TYPE_MOVE_SEND_ONCE)
> + inp->msgh_remote_port = SEND_ONCE_INFO (info)->forward;
> + else
> + {
> + assert (SEND_INFO (info)->receive_right);
> + inp->msgh_remote_port = SEND_INFO (info)->receive_right->forward;
> + }
OK, I'm lost here. Why was the else case not necessary in the original
rpctrace?...
> if (this_type == MACH_MSG_TYPE_MOVE_SEND_ONCE)
> {
> /* We have a message to forward for a send-once wrapper object.
> @@ -822,8 +1314,9 @@ trace_and_forward (mach_msg_header_t *inp,
> mach_msg_header_t *outp)
> we are consuming its `forward' right in the message we send. */
> free (info->name);
> info->name = 0;
> - info->u.nextfree = freelist;
> - freelist = info;
> + SEND_ONCE_INFO (info)->forward = 0;
> + SEND_ONCE_INFO (info)->nextfree = freelist;
> + freelist = SEND_ONCE_INFO (info);
> }
> else
> this_type = MACH_MSG_TYPE_COPY_SEND;
> @@ -839,34 +1332,81 @@ trace_and_forward (mach_msg_header_t *inp,
> mach_msg_header_t *outp)
> if (inp->msgh_local_port == MACH_PORT_NULL
> && info->type == MACH_MSG_TYPE_MOVE_SEND_ONCE
> && inp->msgh_size >= sizeof (mig_reply_header_t)
> + /* The notification message is considered as a request. */
> + && (inp->msgh_id > 72 || inp->msgh_id < 64)
Proper handling of notification messages doesn't seem related to tracing
multiple tasks either, i.e. could go in an extra patch. Or am I missing
something?...
> && (*(int *) &((mig_reply_header_t *) inp)->RetCodeType
> == *(int *)&RetCodeType))
> {
> + struct req_info *req = remove_request (inp->msgh_id - 100,
> + inp->msgh_remote_port);
> + assert (req);
> + req->is_req = FALSE;
> /* This sure looks like an RPC reply message. */
> mig_reply_header_t *rh = (void *) inp;
> - print_reply_header (info, rh);
> + print_reply_header ((struct send_once_info *) info, rh, req);
> putc (' ', ostream);
> - print_contents (&rh->Head, rh + 1);
> + fflush (ostream);
As I already said (and you agreed), the fflush() is not at all related
to tracing multiple tasks. It's a generic improvement that should be
commited independantly.
(Like some other of the generic fixes, it shouldn't even be part of the
patch series; just a totally independant patch...)
> + print_contents (&rh->Head, rh + 1, req);
> putc ('\n', ostream);
>
> if (inp->msgh_id == 2161)/* the reply message for thread_create */
> - wrap_new_thread (inp);
> + wrap_new_thread (inp, req);
> + else if (inp->msgh_id == 2107) /* for task_create */
> + wrap_new_task (inp, req);
> +
> + free (req);
> }
> else
> {
> + struct task_info *task_info;
> + task_t to = 0;
> + struct req_info *req = NULL;
> +
> /* Print something about the message header. */
> - print_request_header (info, inp);
> - print_contents (inp, inp + 1);
> + print_request_header ((struct sender_info *) info, inp);
> + /* It's a nofication message. */
> + if (inp->msgh_id <= 72 && inp->msgh_id >= 64)
> + {
> + assert (info->type == MACH_MSG_TYPE_MOVE_SEND_ONCE);
> + /* mach_notify_port_destroyed message has a port,
> + * TODO how do I handle it? */
> + assert (inp->msgh_id != 69);
> + }
> +
> + /* If it's mach_port RPC,
> + * the port rights in the message will be moved to the target task. */
> + else if (inp->msgh_id >= 3200 && inp->msgh_id <= 3218)
> + to = SEND_INFO (info)->receive_right->forward;
> + else
> + to = SEND_INFO (info)->receive_right->task;
> + if (info->type == MACH_MSG_TYPE_MOVE_SEND)
> + req = add_request (inp->msgh_id, reply_port,
> + SEND_INFO (info)->task, to);
> +
> + /* If it's the notification message, req is NULL.
> + * TODO again, it's difficult to handle mach_notify_port_destroyed */
> + print_contents (inp, inp + 1, req);
> if (inp->msgh_local_port == MACH_PORT_NULL) /* simpleroutine */
> - fprintf (ostream, ");\n");
> + {
> + /* If it's a simpleroutine,
> + * we don't need the request information any more. */
> + req = remove_request (inp->msgh_id, reply_port);
> + free (req);
> + fprintf (ostream, ");\n");
> + }
> else
> /* Leave a partial line that will be finished later. */
> fprintf (ostream, ")");
> + fflush (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);
> + /* If it's the first request from the traced task,
> + * wrap the all threads in the task. */
> + task_info = hurd_ihash_find (&task_ihash, SEND_INFO (info)->task);
> + if (task_info && !task_info->threads_wrapped)
> + {
> + wrap_all_threads (SEND_INFO (info)->task);
> + task_info->threads_wrapped = TRUE;
> + }
Can't you apply this more generic handling already in the patch that
adds the wrap_all_threads() stuff in the first place?
> }
> }
>
> @@ -929,19 +1469,16 @@ static const char *const msg_types[] =
> };
> #endif
>
> -static mach_port_t expected_reply_port;
> -
> static void
> -print_request_header (struct traced_info *receiver, mach_msg_header_t *msg)
> +print_request_header (struct sender_info *receiver, mach_msg_header_t *msg)
> {
> const char *msgname = msgid_name (msg->msgh_id);
>
> - expected_reply_port = msg->msgh_local_port;
> -
> - if (receiver->name != 0)
> - fprintf (ostream, "%4s->", receiver->name);
> + if (TRACED_INFO (receiver)->name != 0)
> + fprintf (ostream, "%4s->", TRACED_INFO (receiver)->name);
> else
> - fprintf (ostream, "%4u->", (unsigned int) receiver->pi.port_right);
> + fprintf (ostream, "%4u->",
> + (unsigned int) TRACED_INFO (receiver)->pi.port_right);
>
> if (msgname != 0)
> fprintf (ostream, "%5s (", msgname);
> @@ -950,57 +1487,17 @@ print_request_header (struct traced_info *receiver,
> mach_msg_header_t *msg)
> }
>
> static void
> -unfinished_line (void)
> +print_reply_header (struct send_once_info *info, mig_reply_header_t *reply,
> + struct req_info *req)
> {
> - /* A partial line was printed by print_request_header, but
> - cannot be finished before we print something else.
> - Finish this line with the name of the reply port that
> - will appear in the disconnected reply later on. */
> - fprintf (ostream, " > %4u ...\n", expected_reply_port);
> -}
Another change I do not understand: why is the unfinished_line()
handling not necessary anymore?...
> -
> -static void
> -print_reply_header (struct traced_info *info, mig_reply_header_t *reply)
> -{
> - if (info->pi.port_right == expected_reply_port)
> - {
> - /* We have printed a partial line for the request message,
> - and now we have the corresponding reply. */
> - if (reply->Head.msgh_id == info->u.send_once.sent_msgid + 100)
> - fprintf (ostream, " = "); /* normal case */
> - else
> - /* This is not the proper reply message ID. */
> - fprintf (ostream, " =(%u != %u) ",
> - reply->Head.msgh_id,
> - info->u.send_once.sent_msgid + 100);
> - }
> + /* We have printed a partial line for the request message,
> + and now we have the corresponding reply. */
> + if (reply->Head.msgh_id == req->req_id + 100)
> + fprintf (ostream, " = "); /* normal case */
> else
> - {
> - /* This does not match up with the last thing printed. */
> - if (expected_reply_port != MACH_PORT_NULL)
> - /* We don't print anything if the last call was a simpleroutine. */
> - unfinished_line ();
> - if (info->name == 0)
> - /* This was not a reply port in previous message sent
> - through our wrappers. */
> - fprintf (ostream, "reply?%4u",
> - (unsigned int) info->pi.port_right);
> - else
> - fprintf (ostream, "%s%4u",
> - info->name, (unsigned int) info->pi.port_right);
> - if (reply->Head.msgh_id == info->u.send_once.sent_msgid + 100)
> - /* This is a normal reply to a previous request. */
> - fprintf (ostream, " > ");
> - else
> - {
> - /* Weirdo. */
> - const char *msgname = msgid_name (reply->Head.msgh_id);
> - if (msgname == 0)
> - fprintf (ostream, " >(%u) ", reply->Head.msgh_id);
> - else
> - fprintf (ostream, " >(%s) ", msgname);
> - }
> - }
> + /* This is not the proper reply message ID. */
> + fprintf (ostream, " =(%u != %u) ",
> + reply->Head.msgh_id, req->req_id + 100);
>
> if (reply->RetCode == 0)
> fprintf (ostream, "0");
> @@ -1012,8 +1509,6 @@ print_reply_header (struct traced_info *info,
> mig_reply_header_t *reply)
> else
> fprintf (ostream, "%#x (%s)", reply->RetCode, str);
> }
> -
> - expected_reply_port = MACH_PORT_NULL;
> }
>
>
> @@ -1108,7 +1603,9 @@ traced_spawn (char **argv, char **envp)
> error_t err;
> pid_t pid;
> mach_port_t task_wrapper;
> - struct traced_info *ti;
> + task_t traced_task;
> + struct sender_info *ti;
> + struct receiver_info *receive_ti;
> file_t file = file_name_path_lookup (argv[0], getenv ("PATH"),
> O_EXEC, 0, 0);
>
> @@ -1122,6 +1619,7 @@ traced_spawn (char **argv, char **envp)
> 0, &traced_task);
> assert_perror (err);
>
> + add_task (traced_task);
> /* Declare the new task to be our child. This is what a fork does. */
> err = proc_child (getproc (), traced_task);
> if (err)
> @@ -1130,9 +1628,12 @@ traced_spawn (char **argv, char **envp)
> if (pid < 0)
> error (2, errno, "task2pid");
>
> + receive_ti = new_receiver_info (traced_task, unknown_task);
> /* Create a trace wrapper for the task port. */
> - ti = new_send_wrapper (traced_task, &task_wrapper);/* consumes ref */
> - asprintf (&ti->name, "task%d", (int) pid);
> + ti = new_send_wrapper (receive_ti, traced_task, &task_wrapper);
> + ti->task = traced_task;
> + free (TRACED_INFO (ti)->name);
> + asprintf (&TRACED_INFO (ti)->name, "task%d(pid%d)", traced_task, pid);
>
> /* Replace the task's kernel port with the wrapper. When this task calls
> `mach_task_self ()', it will get our wrapper send right instead of its
> @@ -1208,6 +1709,7 @@ main (int argc, char **argv, char **envp)
> bool nostdinc = FALSE;
> const char *outfile = 0;
> char **cmd_argv = 0;
> + error_t err;
>
> /* Parse our options... */
> error_t parse_opt (int key, char *arg, struct argp_state *state)
> @@ -1252,6 +1754,10 @@ main (int argc, char **argv, char **envp)
> /* Parse our arguments. */
> argp_parse (&argp, argc, argv, ARGP_IN_ORDER, 0, 0);
>
> + err = mach_port_allocate (mach_task_self (), MACH_PORT_RIGHT_DEAD_NAME,
> + &unknown_task);
Why can't we just use a simple constant for this, like for
UNKNOWN_NAME?...
> + assert_perror (err);
Don't use assert() here -- this call is perfectly capable of failing in
normal operation :-)
> +
> /* Insert the files from STD_MSGIDS_DIR at the beginning of the list, so
> that
> their content can be overridden by subsequently parsed files. */
> if (nostdinc == FALSE)
> @@ -1280,7 +1786,11 @@ main (int argc, char **argv, char **envp)
> setlinebuf (ostream);
>
> traced_bucket = ports_create_bucket ();
> - traced_class = ports_create_class (0, &traced_dropweak);
> + traced_class = ports_create_class (&traced_clean, NULL);
> + other_class = ports_create_class (0, 0);
> + err = ports_create_port (other_class, traced_bucket,
> + sizeof (*notify_pi), ¬ify_pi);
> + assert_perror (err);
Again, no assert() please...
>
> hurd_ihash_set_cleanup (&msgid_ihash, msgid_ihash_cleanup, 0);
>
> @@ -1306,6 +1816,8 @@ main (int argc, char **argv, char **envp)
> else
> fprintf (ostream, "Child %d %s\n", pid, strsignal (WTERMSIG (status)));
> }
> +
And another non-empty blank line.
> + ports_destroy_right (notify_pi);
>
> return 0;
> }
Phew. I've done it! :-)
-antrik-
| [Prev in Thread] |
Current Thread |
[Next in Thread] |
- Re: [PATCH4/4] fully enable rpctrace to trace multitask programs.,
olafBuddenhagen <=