Re: patch to fix process name: omkmigd -> real name

On Tue, Apr 25, 2006 at 10:22:09AM -0600, Matt Dew wrote:
> Here's a patch to fix the process name on the migrated node.  Please
> review.
> 
> If you run top or look in /proc/<pid>/status the name still shows up
> as omkmigd. This patch fixes that.  It calls proc_pid_cmdline to get
> the real name, strips off any preceeding path information to just show
> the executable name and then calls set_task_comm to update the process
> struct. The process will show up on the remode node as it does on the
> home node.

> diff -Naur linux-2.6.16/hpc/migrecv.c linux-2.6.16-om/hpc/migrecv.c
> --- linux-2.6.16/hpc/migrecv.c	2006-04-25 08:27:50.000000000 -0600
> +++ linux-2.6.16-om/hpc/migrecv.c	2006-04-25 08:15:42.000000000 -0600
> @@ -322,6 +322,12 @@
>  	/* link against the other end */
>  	om_link_t *link = (om_link_t *) ptr;
>  	int error;
> +        int i;
> +
> +        char buf[17]; // longest exec name allowed
> +        unsigned int len;
> +        extern int proc_pid_cmdline(struct task_struct *, char *);
> +        extern void reparent_to_init(void);
>  

How did you find 17 here ?
it look to me the cmdline could be as long as PAGE_SIZE, and it's
dependant of mm->arg_end - mm->arg_start

Don't define extern that's extremely evil. if the prototype change I'll
spend hours to track this down. If you want to remove the warning,
define reparent_to_init in some linux header, that *IS* included by
kernel/exit.c. That way the C compiler can know if the prototype are the
same, by verifying the define prototype with the function.

>  	OM_VERBOSE_MIG("[OM] starting process(%d)\n", p->pid);
>  	reparent_to_init();
> +
> +        len=proc_pid_cmdline(p, buf);

proc_pid_cmdline is too generic function to do that. it assume that you
are not even using this mm (as the request can come from any task). Plus
depending more on procfs is not a good thing, since I plan to remove
everything related to proc in some future.

You can easily create a smaller function (in hpc/task.c maybe) that
does the same thing without beeing generic: you don't need to grab the mm,
nor do you need to check that the ->mm is here.
All tasks migrated are task with a mm (only kernel thread doesn't have mm)

It should all boil down to do an access_process_vm with the correct
parameter and check if the task did redefine the title, another
access_process_vm. that would prevent the need to use procfs code.

> +        // strip path info
> +        for  (i=len;i>=0;i--) 
> +           if (buf[i]=='/') 
> +              break;
> +           i++;

OUCH. it's either an indentation error or missing braces.
(it's appear to be the former, but that's confusing)
too much python lately ? :)

> +        set_task_comm(p,buf+i);
> +

You need to fix coding style as well. no space for indentation, space
after ';' and before&after "==" ">=" "="

Cheers,
-- 
Vincent Hanquez


-------------------------------------------------------
Using Tomcat but need to do more? Need to support web services, security?
Get stuff done quickly with pre-integrated technology to make your job easier
Download IBM WebSphere Application Server v.1.0.1 based on Apache Geronimo
http://sel.as-us.falkag.net/sel?cmd=lnk&kid=120709&bid=263057&dat=121642