Skip to contentSkip to conversation

Some useless cleanup

RFC In review Paul P Komkoff Jr 6 participants 9 messages May 9, 2002 – May 13, 2002

Subsystems (16)
Networking Network drivers Memory management VFS +12 more subsystems
lore
Share & tools
lore

Paul P Komkoff Jr

author
#0 lore raw

RFC 1/1 Some useless cleanup

Look at this very funny cleanup changeset for 2.4
avaliable at linux-stingr.bkbits.net/comm

For those who don't have bk I including it here below.

Please give your comments. Maybe it is completely useless and anything should stay as before, or maybe it is somewhat useful and this abstraction will decrease possibility of bugs such as akpm worked around in 8139too

# This is a BitKeeper generated patch for the following project:
# Project Name: Linux kernel tree
# This patch format is intended for GNU patch command version 2.5 or higher. # This patch includes the following deltas:
# ChangeSet 1.417 -> 1.422
# drivers/net/8139too.c 1.26 -> 1.27
# drivers/scsi/scsi_error.c 1.7 -> 1.8
# fs/jffs2/background.c 1.3 -> 1.4

#	    fs/nfsd/nfssvc.c	1.8     -> 1.9    
#	         fs/buffer.c	1.62    -> 1.64   
#	   net/khttpd/main.c	1.3     -> 1.4    
#	drivers/mtd/devices/blkmtd.c	1.3     -> 1.4    
#	fs/reiserfs/journal.c	1.21    -> 1.22   
#	include/linux/sched.h	1.29    -> 1.31   
#	         mm/vmscan.c	1.59    -> 1.60   
#	    fs/jbd/journal.c	1.4     -> 1.5    
#	    kernel/softirq.c	1.8     -> 1.9    
#	drivers/usb/storage/usb.c	1.9     -> 1.10   
#	     drivers/md/md.c	1.30    -> 1.31   
#	drivers/mtd/mtdblock.c	1.8     -> 1.9    
#	           fs/exec.c	1.20    -> 1.22   
#	 fs/lockd/clntlock.c	1.3     -> 1.4    
#	    kernel/context.c	1.4     -> 1.5    
#	   drivers/acpi/os.c	1.8     -> 1.9    
#	   drivers/usb/hub.c	1.15    -> 1.16   
#	  net/sunrpc/sched.c	1.8     -> 1.9    
#	    fs/jffs/intrep.c	1.7     -> 1.8    
#	      fs/lockd/svc.c	1.6     -> 1.7

#
# The following is the BitKeeper ChangeSet Log
# --------------------------------------------
# 02/05/06 [email protected] 1.418
# task_t->comm cleanup: pilot
# --------------------------------------------
# 02/05/07 [email protected] 1.419

# cut down the tails
# --------------------------------------------
# 02/05/07	[email protected]	1.420
# task_t->comm cleanup part 1: replace sprintfs in fs/*
# --------------------------------------------
# 02/05/07	[email protected]	1.421
# task_t->comm cleanup: less redundant dependencies
# --------------------------------------------
# 02/05/08	[email protected]	1.422
# task_t->comm cleanup part 2: drivers, net, and mm
# --------------------------------------------
#
diff -Nru a/drivers/acpi/os.c b/drivers/acpi/os.c
--- a/drivers/acpi/os.c	Thu May  9 14:18:32 2002
+++ b/drivers/acpi/os.c	Thu May  9 14:18:32 2002
@@ -568,7 +568,7 @@
 	PROC_NAME("acpi_os_queue_exec");

daemonize();
- strcpy(current->comm, "kacpidpc");
+ set_current_title("kacpidpc");

if (!dpc || !dpc->function)
return AE_BAD_PARAMETER;

drivers/md/md.c +1 -1
diff -Nru a/drivers/md/md.c b/drivers/md/md.c
@@ -2920,7 +2920,7 @@
 
 	daemonize();
 
-	sprintf(current->comm, thread->name);
+	set_current_title(thread->name);
 	md_init_signals();
 	md_flush_signals();
 	thread->tsk = current;
drivers/mtd/devices/blkmtd.c +1 -1
diff -Nru a/drivers/mtd/devices/blkmtd.c b/drivers/mtd/devices/blkmtd.c
@@ -307,7 +307,7 @@
   DECLARE_WAITQUEUE(wait, tsk);
   DEBUG(1, "blkmtd: writetask: starting (pid = %d)\n", tsk->pid);
   daemonize();
-  strcpy(tsk->comm, "blkmtdd");
+  set_task_title(tsk, "blkmtdd");
   tsk->tty = NULL;
   spin_lock_irq(&tsk->sigmask_lock);
   sigfillset(&tsk->blocked);
drivers/mtd/mtdblock.c +1 -1
diff -Nru a/drivers/mtd/mtdblock.c b/drivers/mtd/mtdblock.c
@@ -487,7 +487,7 @@
 	tsk->pgrp = 1;
 	/* we might get involved when memory gets low, so use PF_MEMALLOC */
 	tsk->flags |= PF_MEMALLOC;
-	strcpy(tsk->comm, "mtdblockd");
+	set_task_title(tsk, "mtdblockd");
 	tsk->tty = NULL;
 	spin_lock_irq(&tsk->sigmask_lock);
 	sigfillset(&tsk->blocked);
drivers/net/8139too.c +1 -2
diff -Nru a/drivers/net/8139too.c b/drivers/net/8139too.c
@@ -1563,8 +1563,7 @@
 	recalc_sigpending(current);
 	spin_unlock_irq(&current->sigmask_lock);
 
-	strncpy (current->comm, dev->name, sizeof(current->comm) - 1);
-	current->comm[sizeof(current->comm) - 1] = '\0';
+	set_current_title(dev->name);
 
 	while (1) {
 		timeout = next_tick;
drivers/scsi/scsi_error.c +1 -1
diff -Nru a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
@@ -1873,7 +1873,7 @@
 	 * Set the name of this process.
 	 */
 
-	sprintf(current->comm, "scsi_eh_%d", host->host_no);
+	set_current_title("scsi_eh_%d", host->host_no);
 
 	host->eh_wait = &sem;
 	host->ehandler = current;
drivers/usb/hub.c +1 -1
diff -Nru a/drivers/usb/hub.c b/drivers/usb/hub.c
@@ -907,7 +907,7 @@
 	reparent_to_init();
 
 	/* Setup a nice name */
-	strcpy(current->comm, "khubd");
+	set_current_title("khubd");
 
 	/* Send me a signal to get me die (for debugging) */
 	do {
drivers/usb/storage/usb.c +1 -1
diff -Nru a/drivers/usb/storage/usb.c b/drivers/usb/storage/usb.c
@@ -328,7 +328,7 @@
 	spin_unlock_irq(&current->sigmask_lock);
 
 	/* set our name for identification purposes */
-	sprintf(current->comm, "usb-storage-%d", us->host_number);
+	set_current_title("usb-storage-%d", us->host_number);
 
 	unlock_kernel();
fs/buffer.c +2 -2
diff -Nru a/fs/buffer.c b/fs/buffer.c
@@ -2978,7 +2978,7 @@
 
 	tsk->session = 1;
 	tsk->pgrp = 1;
-	strcpy(tsk->comm, "bdflush");
+	set_task_title(tsk, "bdflush");
 
 	/* avoid getting signals */
 	spin_lock_irq(&tsk->sigmask_lock);

@@ -3028,7 +3028,7 @@
 
 	tsk->session = 1;
 	tsk->pgrp = 1;
-	strcpy(tsk->comm, "kupdated");
+	set_task_title(tsk, "kupdated");
 
 	/* sigstop and sigcont will stop and wakeup kupdate */
 	spin_lock_irq(&tsk->sigmask_lock);
fs/exec.c +8 -9
diff -Nru a/fs/exec.c b/fs/exec.c
@@ -35,6 +35,7 @@
 #include <linux/highmem.h>
 #include <linux/spinlock.h>
 #include <linux/personality.h>
+#include <linux/compiler.h>
 #define __NO_VERSION__
 #include <linux/module.h>
 

@@ -554,15 +555,13 @@
 
 	if (current->euid == current->uid && current->egid == current->gid)
 		current->mm->dumpable = 1;
-	name = bprm->filename;
-	for (i=0; (ch = *(name++)) != '\0';) {
-		if (ch == '/')
-			i = 0;
-		else
-			if (i < 15)
-				current->comm[i++] = ch;
-	}
-	current->comm[i] = '\0';
+
+	if (likely(name = strrchr(bprm->filename, '/')))
+		name++;
+	else
+		name = bprm->filename;
+
+	set_current_title(name);
 
 	flush_thread();
fs/jbd/journal.c +1 -1
diff -Nru a/fs/jbd/journal.c b/fs/jbd/journal.c
@@ -210,7 +210,7 @@
 	recalc_sigpending(current);
 	spin_unlock_irq(&current->sigmask_lock);
 
-	sprintf(current->comm, "kjournald");
+	set_current_title("kjournald");
 
 	/* Set up an interval timer which can be used to trigger a
            commit wakeup after the commit interval expires */
fs/jffs/intrep.c +1 -1
diff -Nru a/fs/jffs/intrep.c b/fs/jffs/intrep.c
@@ -3354,7 +3354,7 @@
 	siginitsetinv (&current->blocked, sigmask(SIGHUP) | sigmask(SIGKILL) | sigmask(SIGSTOP) | sigmask(SIGCONT));
 	recalc_sigpending(current);
 	spin_unlock_irq(&current->sigmask_lock);
-	strcpy(current->comm, "jffs_gcd");
+	set_current_title("jffs_gcd");
 
 	D1(printk (KERN_NOTICE "jffs_garbage_collect_thread(): Starting infinite loop.\n"));
fs/jffs2/background.c +1 -1
diff -Nru a/fs/jffs2/background.c b/fs/jffs2/background.c
@@ -104,7 +104,7 @@
 	c->gc_task = current;
 	up(&c->gc_thread_start);
 
-        sprintf(current->comm, "jffs2_gcd_mtd%d", c->mtd->index);
+        set_current_title("jffs2_gcd_mtd%d", c->mtd->index);
 
 	/* FIXME in the 2.2 backport */
 	current->nice = 10;
fs/lockd/clntlock.c +1 -3
diff -Nru a/fs/lockd/clntlock.c b/fs/lockd/clntlock.c
@@ -203,9 +203,7 @@
 
 	daemonize();
 	reparent_to_init();
-	snprintf(current->comm, sizeof(current->comm),
-		 "%s-reclaim",
-		 host->h_name);
+	set_current_title("%s-reclaim", host->h_name);
 
 	/* This one ensures that our parent doesn't terminate while the
 	 * reclaim is in progress */
fs/lockd/svc.c +1 -1
diff -Nru a/fs/lockd/svc.c b/fs/lockd/svc.c
@@ -99,7 +99,7 @@
 
 	daemonize();
 	reparent_to_init();
-	sprintf(current->comm, "lockd");
+	set_current_title("lockd");
 
 	/* Process request with signals blocked.  */
 	spin_lock_irq(&current->sigmask_lock);
fs/nfsd/nfssvc.c +1 -1
diff -Nru a/fs/nfsd/nfssvc.c b/fs/nfsd/nfssvc.c
@@ -161,7 +161,7 @@
 	MOD_INC_USE_COUNT;
 	lock_kernel();
 	daemonize();
-	sprintf(current->comm, "nfsd");
+	set_current_title("nfsd");
 	current->rlim[RLIMIT_FSIZE].rlim_cur = RLIM_INFINITY;
 
 	nfsdstats.th_cnt++;
fs/reiserfs/journal.c +1 -1
diff -Nru a/fs/reiserfs/journal.c b/fs/reiserfs/journal.c
@@ -1872,7 +1872,7 @@
   recalc_sigpending(current);
   spin_unlock_irq(&current->sigmask_lock);
 
-  sprintf(current->comm, "kreiserfsd") ;
+  set_current_title("kreiserfsd") ;
   lock_kernel() ;
   while(1) {
include/linux/sched.h +10 -1
diff -Nru a/include/linux/sched.h b/include/linux/sched.h
@@ -110,6 +110,15 @@
 	__set_current_state(state_value)
 #endif
 
+#define TASK_TITLE_MAX	15
+#define __TASK_TITLE_SZ	(TASK_TITLE_MAX + 1)
+
+#define set_task_title(tsk, arg...) \
+	snprintf(tsk->comm, sizeof(tsk->comm), ##arg)
+
+#define set_current_title(arg...) \
+	snprintf(current->comm, sizeof(current->comm), ##arg)
+
 /*
  * Scheduling policies
  */

@@ -379,7 +388,7 @@
 /* limits */
 	struct rlimit rlim[RLIM_NLIMITS];
 	unsigned short used_math;
-	char comm[16];
+	char comm[__TASK_TITLE_SZ];
 /* file system info */
 	int link_count, total_link_count;
 	struct tty_struct *tty; /* NULL if no tty */
kernel/context.c +1 -1
diff -Nru a/kernel/context.c b/kernel/context.c
@@ -71,7 +71,7 @@
 	struct k_sigaction sa;
 
 	daemonize();
-	strcpy(curtask->comm, "keventd");
+	set_task_title(curtask, "keventd");
 	keventd_running = 1;
 	keventd_task = curtask;
kernel/softirq.c +1 -1
diff -Nru a/kernel/softirq.c b/kernel/softirq.c
@@ -373,7 +373,7 @@
 	while (smp_processor_id() != cpu)
 		schedule();
 
-	sprintf(current->comm, "ksoftirqd_CPU%d", bind_cpu);
+	set_current_title("ksoftirqd_CPU%d", bind_cpu);
 
 	__set_current_state(TASK_INTERRUPTIBLE);
 	mb();
mm/vmscan.c +1 -1
diff -Nru a/mm/vmscan.c b/mm/vmscan.c
@@ -704,7 +704,7 @@
 	DECLARE_WAITQUEUE(wait, tsk);
 
 	daemonize();
-	strcpy(tsk->comm, "kswapd");
+	set_task_title(tsk, "kswapd");
 	sigfillset(&tsk->blocked);
 	
 	/*
net/khttpd/main.c +2 -2
diff -Nru a/net/khttpd/main.c b/net/khttpd/main.c
@@ -105,7 +105,7 @@
 	if (cpu_pointer!=NULL)
 	CPUNR=(int)*(int*)cpu_pointer;
 
-	sprintf(current->comm,"khttpd - %i",CPUNR);
+	set_current_title("khttpd - %i", CPUNR);
 	daemonize();
 	
 	init_waitqueue_head(&(DummyWQ[CPUNR]));

@@ -195,7 +195,7 @@
 	DECLARE_WAIT_QUEUE_HEAD(WQ);
 	
 	
-	sprintf(current->comm,"khttpd manager");
+	set_current_title("khttpd manager");
 	daemonize();
net/sunrpc/sched.c +1 -1
diff -Nru a/net/sunrpc/sched.c b/net/sunrpc/sched.c
@@ -1067,7 +1067,7 @@
 	recalc_sigpending(current);
 	spin_unlock_irq(&current->sigmask_lock);
 
-	strcpy(current->comm, "rpciod");
+	set_current_title("rpciod");
 
 	dprintk("RPC: rpciod starting (pid %d)\n", rpciod_pid);
 	while (rpciod_users) {
Signature · 2 lines
Paul P 'Stingray' Komkoff 'Greatest' Jr // (icq)23200764 // (irc)Spacebar
  PPKJ1-RIPE // (smtp)[email protected] // (http)stingr.net // (pgp)0xA4B4ECA4
#1 lore raw
Quoted 8 lines · Paul P Komkoff Jr

Look at this very funny cleanup changeset for 2.4
avaliable at linux-stingr.bkbits.net/comm

For those who don't have bk I including it here below.

Please give your comments. Maybe it is completely useless and anything should stay as before, or maybe it is somewhat useful and this abstraction will decrease possibility of bugs such as akpm worked around in 8139too

Well, it isn't bad, but what's the point in multiple set_xxxxxx(char *dst, char *src) functions?

Maybe it makes more sense to have a generic macro which copies string into char[N] buffer, avoiding overflow.

Actually, there is similar code in your mail:

Quoted 3 lines

-       strncpy (current->comm, dev->name, sizeof(current->comm) - 1); -       current->comm[sizeof(current->comm) - 1] = '\0';
+       set_current_title(dev->name);

A macro:

#define STRNCPY(dst,src) \
	do { \
		/* todo: put clever check that dst is char[] here */ \
		strncpy((dst), (src), sizeof(dst)-1); \
		dst[sizeof(dst)-1] = '\0'; \
	} while(0)

--
vda

author
#2 lore raw
In reply to Paul P Komkoff Jr

Replying to Denis Vlasenko:

Quoted 6 lines

Well, it isn't bad, but what's the point in multiple set_xxxxxx(char *dst, char *src) functions?

Maybe it makes more sense to have a generic macro which copies string into char[N] buffer, avoiding overflow.

Generic plan. I has something in mind when asked, but not this ...

Actually in task_t.comm we have 2 cases
1. strncpy(a, b, sz)
2. snprintf(a, n, blah-blah...)

I thought somebody will beat me for completely eliminating strcpyn and replacing it with snprintf in ALL CASES which is more expensive.

Quoted 8 lines

A macro:

#define STRNCPY(dst,src) \
	do { \
		/* todo: put clever check that dst is char[] here */ \
		strncpy((dst), (src), sizeof(dst)-1); \
		dst[sizeof(dst)-1] = '\0'; \
	} while(0)

Abstracting .comm access can result in, finally, replacing comm[16] with, for example, *comm

Or if we require to do match_comm (netfilter match, connections belong to process specified by name) job we can patchhook somewhere in set_xxx to avoid excessive for_each_tasked strcmps.

... more?

Signature · 2 lines
Paul P 'Stingray' Komkoff 'Greatest' Jr // (icq)23200764 // (irc)Spacebar
  PPKJ1-RIPE // (smtp)[email protected] // (http)stingr.net // (pgp)0xA4B4ECA4
#3 lore raw
In reply to Paul P Komkoff Jr
Quoted 2 lines · On Thu, 9 May 2002 14:28:41 +0400 Paul P Komkoff Jr <[email protected]> wrote:

Look at this very funny cleanup changeset for 2.4
avaliable at linux-stingr.bkbits.net/comm

Um, why not simply:

static inline void set_name(struct task_struct *tsk, const char *name)

{
	/* comm is always nul-terminated already */
	strncpy(tsk->comm, name, sizeof(tsk->comm)-1);
}

Your implementation using snprintf is (wasteful and) dangerous, Rusty.

Signature · 2 lines
   there are those who do and those who hang on and you don't see too
   many doers quoting their contemporaries.  -- Larry McVoy
#4 lore raw
In reply to Paul P Komkoff Jr
Quoted 10 lines · Rusty Russell

Um, why not simply:

static inline void set_name(struct task_struct *tsk, const char *name)

{
	/* comm is always nul-terminated already */
	strncpy(tsk->comm, name, sizeof(tsk->comm)-1);
}

Your implementation using snprintf is (wasteful and) dangerous, Rusty.

And both implementations suffer from the fact that if tsk->comm were to change from a fixed length array to a char*, allowing arbitrarily sized names, you would end up copying very little indeed. :) What not something more general like:

char * safe_strncpy(char *dst, const char *src, size_t size)

{
    dst[size-1] = '\0';
    strncpy(dst, src, size-1);
}

-Erik

--
Erik B. Andersen http://codepoet-consulting.com/
--This message was written using 73% post-consumer electrons--

#5 lore raw
Quoted 5 lines · Erik Andersen

char * safe_strncpy(char *dst, const char *src, size_t size)

{
   dst[size-1] = '\0';
   strncpy(dst, src, size-1);
}

Maybe just call it strlcpy() similar to others :
e.g. http://www.tac.eu.org/cgi-bin/man-cgi?strlcpy+3

bernd

Signature · 4 lines
Bernd Petrovitsch                              Email : [email protected]
g.a.m.s gmbh                                  Fax : +43 1 205255-900
Prinz-Eugen-Straße 8                    A-1040 Vienna/Austria/Europe
                     LUGA : http://www.luga.at
#6 lore raw
In reply to Erik Andersen
Quoted 17 lines · Erik Andersen
Quoted 10 lines · Rusty Russell

Um, why not simply:

static inline void set_name(struct task_struct *tsk, const char *name)

{
	/* comm is always nul-terminated already */
	strncpy(tsk->comm, name, sizeof(tsk->comm)-1);
}

Your implementation using snprintf is (wasteful and) dangerous, Rusty.

And both implementations suffer from the fact that if tsk->comm were to change from a fixed length array to a char*, allowing arbitrarily sized names, you would end up copying very little indeed. :) What not something more general like:

Quoted 5 lines

char * safe_strncpy(char *dst, const char *src, size_t size)

{
    dst[size-1] = '\0';
    strncpy(dst, src, size-1);
}

If we're gonna do that, we might as well adopt OpenBSD-style strlcpy and strlcat:

http://www.courtesan.com/todd/papers/strlcpy.html

Signature · 1 line
 "Love the dolphins," she advised him. "Write by W.A.S.T.E.."
#7 lore raw
In reply to Erik Andersen

In message <[email protected]> you write:

Quoted 9 lines · Rusty Russell
Quoted 8 lines

Um, why not simply:

static inline void set_name(struct task_struct *tsk, const char *name)

{
	/* comm is always nul-terminated already */
	strncpy(tsk->comm, name, sizeof(tsk->comm)-1);
}
Quoted 7 lines
Quoted 2 lines

Your implementation using snprintf is (wasteful and) dangerous, Rusty.

And both implementations suffer from the fact that if tsk->comm were to change from a fixed length array to a char*, allowing arbitrarily sized names, you would end up copying very little indeed. :)

Um, yes, if someone were to make a random change to the kernel without looking at what it would effect, the kernel would likely break.

Rusty.
--
Anyone who quotes me in their sig is an idiot. -- Rusty Russell.

author
#8 lore raw
In reply to Paul P Komkoff Jr

Replying to Rusty Russell:

Quoted 1 line

Your implementation using snprintf is (wasteful and) dangerous,

Some pieces inside kernel using formatted titles
for example softirqd_CPU%d
or scsi_eh_%d

(But if we eliminate them then it can end with strncpy)

Signature · 2 lines
Paul P 'Stingray' Komkoff 'Greatest' Jr // (icq)23200764 // (irc)Spacebar
  PPKJ1-RIPE // (smtp)[email protected] // (http)stingr.net // (pgp)0xA4B4ECA4