checking return values for pfiles.stp

checking return values for pfiles.stp

[Erick Tryzelaar]

[-- Attachment #1: Type: text/plain, Size: 320 bytes --]

Good morning,

jistone on #systemtap helped me clean up pfiles.stp to make sure to
check return values when running pfiles.stp, which I've attached.
Also, would there be any possibility to promote any of pfiles's
functionality into a tapset? I find it pretty useful to be able to get
a filename for a given fd.

Thanks!

[-- Attachment #2: 0001-Rewrite-pfiles-to-check-return-values.patch --]
[-- Type: text/x-patch, Size: 10829 bytes --]

From f9af2b7b9cf4d9e58581d8712da39280bcf6f7f5 Mon Sep 17 00:00:00 2001
From: Erick Tryzelaar <erick.tryzelaar@gmail.com>
Date: Tue, 19 Oct 2010 11:40:17 -0700
Subject: [PATCH] Rewrite pfiles to check return values

---
 testsuite/systemtap.examples/process/pfiles.stp |  218 +++++++++++++----------
 1 files changed, 126 insertions(+), 92 deletions(-)

diff --git a/testsuite/systemtap.examples/process/pfiles.stp b/testsuite/systemtap.examples/process/pfiles.stp
index e0a923b..3dd1dc7 100755
--- a/testsuite/systemtap.examples/process/pfiles.stp
+++ b/testsuite/systemtap.examples/process/pfiles.stp
@@ -85,20 +85,18 @@
 
 function task_valid_file_handle:long (task:long, fd:long) %{ /* pure */
 	struct task_struct *p = (struct task_struct *)((long)THIS->task);
-	struct files_struct *files = kread(&p->files);
+	struct files_struct *files;
 	struct file *filp;
 
 	rcu_read_lock();
-	filp = fcheck_files(files, THIS->fd);
-	if (!filp) {
-		THIS->__retvalue = 0;
-		rcu_read_unlock();
-		return;
+	if ((files = kread(&p->files))) {
+		if ((filp = fcheck_files(files, THIS->fd))) {
+			THIS->__retvalue = (long)filp;
+		}
 	}
-	THIS->__retvalue = (long)filp;
-	rcu_read_unlock();
 
 	CATCH_DEREF_FAULT();
+	rcu_read_unlock();
 %}
 
 function i_mode2str:string (i_mode:long) %{ /* pure */
@@ -116,203 +114,239 @@ function i_mode2str:string (i_mode:long) %{ /* pure */
 
 function task_file_handle_i_mode:long (task:long, fd:long) %{ /* pure */
 	struct task_struct *p = (struct task_struct *)((long)THIS->task);
-	struct files_struct *files = kread(&p->files);
+	struct files_struct *files;
 	struct file *filp;
 
 	rcu_read_lock();
-	filp = fcheck_files(files, THIS->fd);
-	THIS->__retvalue = kread(&filp->f_dentry->d_inode->i_mode);
-	rcu_read_unlock();
+	if ((files = kread(&p->files))) {
+		if ((filp = fcheck_files(files, THIS->fd))) {
+			THIS->__retvalue = kread(&filp->f_dentry->d_inode->i_mode);
+		}
+	}
 
 	CATCH_DEREF_FAULT();
+	rcu_read_unlock();
 %}
 
 function task_file_handle_majmin_dev:string (task:long, fd:long) %{ /* pure */
 	struct task_struct *p = (struct task_struct *)((long)THIS->task);
-	struct files_struct *files = kread(&p->files);
+	struct files_struct *files;
 	struct file *filp;
 	int dev_nr;
 
 	rcu_read_lock();
-	filp = fcheck_files(files, THIS->fd);
-	dev_nr = kread(&filp->f_dentry->d_inode->i_sb->s_dev);
-	snprintf(THIS->__retvalue, MAXSTRINGLEN,
-		"%d,%d", MAJOR(dev_nr), MINOR(dev_nr));
-	rcu_read_unlock();
+	if ((files = kread(&p->files))) {
+		if ((filp = fcheck_files(files, THIS->fd))) {
+			dev_nr = kread(&filp->f_dentry->d_inode->i_sb->s_dev);
+			snprintf(THIS->__retvalue, MAXSTRINGLEN,
+				"%d,%d", MAJOR(dev_nr), MINOR(dev_nr));
+		}
+	}
 
 	CATCH_DEREF_FAULT();
+	rcu_read_unlock();
 %}
 
 function task_file_handle_majmin_rdev:string (task:long, fd:long) %{ /* pure */
 	struct task_struct *p = (struct task_struct *)((long)THIS->task);
-	struct files_struct *files = kread(&p->files);
+	struct files_struct *files;
 	struct file *filp;
 	int rdev_nr;
 
 	rcu_read_lock();
-	filp = fcheck_files(files, THIS->fd);
-	rdev_nr = kread(&filp->f_dentry->d_inode->i_rdev);
-	snprintf(THIS->__retvalue, MAXSTRINGLEN,
-		"%d,%d", MAJOR(rdev_nr), MINOR(rdev_nr));
-	rcu_read_unlock();
+	if ((files = kread(&p->files))) {
+		if ((filp = fcheck_files(files, THIS->fd))) {
+			rdev_nr = kread(&filp->f_dentry->d_inode->i_rdev);
+			snprintf(THIS->__retvalue, MAXSTRINGLEN,
+				"%d,%d", MAJOR(rdev_nr), MINOR(rdev_nr));
+		}
+	}
 
 	CATCH_DEREF_FAULT();	
+	rcu_read_unlock();
 %}
 
 function task_file_handle_i_node:long (task:long, fd:long) %{ /* pure */
 	struct task_struct *p = (struct task_struct *)((long)THIS->task);
-	struct files_struct *files = kread(&p->files);
+	struct files_struct *files;
 	struct file *filp;
 	
 	rcu_read_lock();
-	filp = fcheck_files(files, THIS->fd);
-	THIS->__retvalue = kread(&filp->f_dentry->d_inode->i_ino);
-	rcu_read_unlock();
+	if ((files = kread(&p->files))) {
+		if ((filp = fcheck_files(files, THIS->fd))) {
+			THIS->__retvalue = kread(&filp->f_dentry->d_inode->i_ino);
+		}
+	}
 
 	CATCH_DEREF_FAULT();
+	rcu_read_unlock();
 %}
 
 function task_file_handle_uid:long (task:long, fd:long) %{ /* pure */
 	struct task_struct *p = (struct task_struct *)((long)THIS->task);
-	struct files_struct *files = kread(&p->files);
+	struct files_struct *files;
 	struct file *filp;
 	
 	rcu_read_lock();
-	filp = fcheck_files(files, THIS->fd);
+	if ((files = kread(&p->files))) {
+		if ((filp = fcheck_files(files, THIS->fd))) {
 #if LINUX_VERSION_CODE >= KERNEL_VERSION(2,6,29)
-	/* git commit d76b0d9b */
-	THIS->__retvalue = kread(&filp->f_cred->fsuid);
+			/* git commit d76b0d9b */
+			THIS->__retvalue = kread(&filp->f_cred->fsuid);
 #else
-	THIS->__retvalue = kread(&filp->f_uid);
+			THIS->__retvalue = kread(&filp->f_uid);
 #endif
-	rcu_read_unlock();
+		}
+	}
 
 	CATCH_DEREF_FAULT();
+	rcu_read_unlock();
 %}
 
 function task_file_handle_gid:long (task:long, fd:long) %{ /* pure */
 	struct task_struct *p = (struct task_struct *)((long)THIS->task);
-	struct files_struct *files = kread(&p->files);
+	struct files_struct *files;
 	struct file *filp;
 	
 	rcu_read_lock();
-	filp = fcheck_files(files, THIS->fd);
+	if ((files = kread(&p->files))) {
+		if ((filp = fcheck_files(files, THIS->fd))) {
 #if LINUX_VERSION_CODE >= KERNEL_VERSION(2,6,29)
-	/* git commit d76b0d9b */
-	THIS->__retvalue = kread(&filp->f_cred->fsgid);
+			/* git commit d76b0d9b */
+			THIS->__retvalue = kread(&filp->f_cred->fsgid);
 #else
-	THIS->__retvalue = kread(&filp->f_gid);
+			THIS->__retvalue = kread(&filp->f_gid);
 #endif
-	rcu_read_unlock();
+		}
+	}
 
 	CATCH_DEREF_FAULT();
+	rcu_read_unlock();
 %}
 
 function task_file_handle_f_flags:long (task:long, fd:long) %{ /* pure */
 	struct task_struct *p = (struct task_struct *)((long)THIS->task);
-	struct files_struct *files = kread(&p->files);
+	struct files_struct *files;
 	struct file *filp;
 	
 	rcu_read_lock();
-	filp = fcheck_files(files, THIS->fd);
-	THIS->__retvalue = kread(&filp->f_flags);
-	rcu_read_unlock();
+	if ((files = kread(&p->files))) {
+		if ((filp = fcheck_files(files, THIS->fd))) {
+			THIS->__retvalue = kread(&filp->f_flags);
+		}
+	}
 
 	CATCH_DEREF_FAULT();
+	rcu_read_unlock();
 %}
 
 function task_file_handle_fd_flags:string (task:long, fd:long) %{ /* pure */
 	struct task_struct *p = (struct task_struct *)((long)THIS->task);
-	struct files_struct *files = kread(&p->files);
+	struct files_struct *files;
 	struct fdtable *fdt;
 	int gcoe;
 	
 	rcu_read_lock();
-	fdt = files_fdtable(files);
-	gcoe = FD_ISSET(THIS->fd, kread(&fdt->close_on_exec));
-	snprintf(THIS->__retvalue, MAXSTRINGLEN,
-		"%s", gcoe ? "FD_CLOEXEC" : "");
-	rcu_read_unlock();
+	if ((files = kread(&p->files))) {
+		if ((fdt = files_fdtable(files))) {
+			gcoe = FD_ISSET(THIS->fd, kread(&fdt->close_on_exec));
+			snprintf(THIS->__retvalue, MAXSTRINGLEN,
+				"%s", gcoe ? "FD_CLOEXEC" : "");
+		}
+	}
 
 	CATCH_DEREF_FAULT();
+	rcu_read_unlock();
 %}
 
 function task_file_handle_flock:string (task:long, fd:long) %{ /* pure */
 	struct task_struct *p = (struct task_struct *)((long)THIS->task);
-	struct files_struct *files = kread(&p->files);
+	struct files_struct *files;
 	struct file_lock *flock;
 	int fl_type, fl_pid;
 	struct file *filp;
 
 	rcu_read_lock();
-	filp = fcheck_files(files, THIS->fd);
-	flock = kread(&filp->f_dentry->d_inode->i_flock);
-	fl_type = fl_pid = -1;
-	if (flock) {
-		fl_type = kread(&flock->fl_type);
-		fl_pid = kread(&flock->fl_pid);
-	}
+	if ((files = kread(&p->files))) {
+		if ((filp = fcheck_files(files, THIS->fd))) {
+			if ((flock = kread(&filp->f_dentry->d_inode->i_flock))) {
+				fl_type = kread(&flock->fl_type);
+				fl_pid = kread(&flock->fl_pid);
+			} else {
+				fl_type = -1;
+				fl_pid = -1;
+			}
 
-	if (fl_type != -1) /* !flock */
-		snprintf(THIS->__retvalue, MAXSTRINGLEN,
-			"      advisory %s lock set by process %d",
-			fl_type ? "write" : "read", fl_pid);
-	else
-		snprintf(THIS->__retvalue, MAXSTRINGLEN, "NULL");
-	rcu_read_unlock();
+			if (fl_type != -1) { /* !flock */
+				snprintf(THIS->__retvalue, MAXSTRINGLEN,
+					"      advisory %s lock set by process %d",
+					fl_type ? "write" : "read", fl_pid);
+			} else {
+				snprintf(THIS->__retvalue, MAXSTRINGLEN, "NULL");
+			}
+		}
+	}
 
 	CATCH_DEREF_FAULT();
+	rcu_read_unlock();
 %}
 
 function task_file_handle_d_path:string (task:long, fd:long) %{ /* pure */
 	struct task_struct *p = (struct task_struct *)((long)THIS->task);
-	struct files_struct *files = kread(&p->files);
-	char *page = (char *)__get_free_page(GFP_KERNEL);
+	struct files_struct *files;
+	char *page = NULL;
 	struct file *filp;
 	struct dentry *dentry;
 	struct vfsmount *vfsmnt;
-
-	/* TODO: handle !page */
-	if (!page)
-		return;
+	char *path = NULL;
 
 	rcu_read_lock();
-	filp = fcheck_files(files, THIS->fd);
-	dentry = kread(&filp->f_dentry);
-	vfsmnt = kread(&filp->f_vfsmnt);
+	if ((files = kread(&p->files))) {
+		/* TODO: handle !page */
+		if ((page = (char *)__get_free_page(GFP_KERNEL))) {
+			if ((filp = fcheck_files(files, THIS->fd))) {
+
 #if LINUX_VERSION_CODE >= KERNEL_VERSION(2,6,26)
-	/* git commit 9d1bc601 */
-    snprintf(THIS->__retvalue, MAXSTRINGLEN, "%s",
-        d_path(&filp->f_path, page, PAGE_SIZE));
+				/* git commit 9d1bc601 */
+				path = d_path(&filp->f_path, page, PAGE_SIZE);
 #else
-	snprintf(THIS->__retvalue, MAXSTRINGLEN, "%s",
-		d_path(dentry, vfsmnt, page, PAGE_SIZE));
-#endif	
-	free_page((unsigned long)page);
-	rcu_read_unlock();
+				dentry = kread(&filp->f_dentry);
+				vfsmnt = kread(&filp->f_vfsmnt);
 
+				if (dentry && vfsmnt) {
+					path = d_path(dentry, vfsmnt, page, PAGE_SIZE);
+				}
+#endif
+				if (path && !IS_ERR(path)) {
+					snprintf(THIS->__retvalue, MAXSTRINGLEN, "%s", path);
+				}
+			}
+		}
+	}
 	CATCH_DEREF_FAULT();
+
+	if (page) free_page((unsigned long)page);
+
+	rcu_read_unlock();
 %}
 
 function task_file_handle_socket:long (task:long, fd:long) %{ /* pure */
 	struct task_struct *p = (struct task_struct *)((long)THIS->task);
-	struct files_struct *files = kread(&p->files);
+	struct files_struct *files;
 	struct inode *inode;
 	struct file *filp;
 
 	rcu_read_lock();
-	filp = fcheck_files(files, THIS->fd);
-	if (!filp) {
-		THIS->__retvalue = 0;
-	    rcu_read_unlock();
-		return;
+	if ((files = kread(&p->files))) {
+		if ((filp = fcheck_files(files, THIS->fd))) {
+			inode = kread(&filp->f_dentry->d_inode);
+			if (inode && S_ISSOCK(kread(&inode->i_mode)))
+				THIS->__retvalue = (long)SOCKET_I(inode);
+		}
 	}
-	inode = kread(&filp->f_dentry->d_inode);
-	if (S_ISSOCK(kread(&inode->i_mode)))
-		THIS->__retvalue = (long)SOCKET_I(inode);
-	rcu_read_unlock();
 
 	CATCH_DEREF_FAULT();
+	rcu_read_unlock();
 %}
 
 function socket_userlocks:string (sock:long) %{ /* pure */
-- 
1.7.2.1

Re: checking return values for pfiles.stp

[Erick Tryzelaar]

[-- Attachment #1: Type: text/plain, Size: 460 bytes --]

Updated with jistone's suggestions.

On Tue, Oct 19, 2010 at 11:49 AM, Erick Tryzelaar
<erick.tryzelaar@gmail.com> wrote:
> Good morning,
>
> jistone on #systemtap helped me clean up pfiles.stp to make sure to
> check return values when running pfiles.stp, which I've attached.
> Also, would there be any possibility to promote any of pfiles's
> functionality into a tapset? I find it pretty useful to be able to get
> a filename for a given fd.
>
> Thanks!
>

[-- Attachment #2: 0001-Rewrite-pfiles-to-check-return-values.patch --]
[-- Type: text/x-patch, Size: 12360 bytes --]

From f7a5a108b524bdceaca09473b6f7c1666e6c864f Mon Sep 17 00:00:00 2001
From: Erick Tryzelaar <erick.tryzelaar@gmail.com>
Date: Tue, 19 Oct 2010 17:04:00 -0700
Subject: [PATCH] Rewrite pfiles to check return values
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="------------1.7.2.1"

This is a multi-part message in MIME format.
--------------1.7.2.1
Content-Type: text/plain; charset=UTF-8; format=fixed
Content-Transfer-Encoding: 8bit

---
 testsuite/systemtap.examples/process/pfiles.stp |  239 ++++++++++++++---------
 1 files changed, 145 insertions(+), 94 deletions(-)


--------------1.7.2.1
Content-Type: text/x-patch; name="0001-Rewrite-pfiles-to-check-return-values.patch"
Content-Transfer-Encoding: 8bit
Content-Disposition: attachment; filename="0001-Rewrite-pfiles-to-check-return-values.patch"

diff --git a/testsuite/systemtap.examples/process/pfiles.stp b/testsuite/systemtap.examples/process/pfiles.stp
index e0a923b..9b52fd3 100755
--- a/testsuite/systemtap.examples/process/pfiles.stp
+++ b/testsuite/systemtap.examples/process/pfiles.stp
@@ -85,20 +85,17 @@
 
 function task_valid_file_handle:long (task:long, fd:long) %{ /* pure */
 	struct task_struct *p = (struct task_struct *)((long)THIS->task);
-	struct files_struct *files = kread(&p->files);
+	struct files_struct *files;
 	struct file *filp;
 
 	rcu_read_lock();
-	filp = fcheck_files(files, THIS->fd);
-	if (!filp) {
-		THIS->__retvalue = 0;
-		rcu_read_unlock();
-		return;
+	if ((files = kread(&p->files)) &&
+	    (filp = fcheck_files(files, THIS->fd))) {
+		THIS->__retvalue = (long)filp;
 	}
-	THIS->__retvalue = (long)filp;
-	rcu_read_unlock();
 
 	CATCH_DEREF_FAULT();
+	rcu_read_unlock();
 %}
 
 function i_mode2str:string (i_mode:long) %{ /* pure */
@@ -116,203 +113,256 @@ function i_mode2str:string (i_mode:long) %{ /* pure */
 
 function task_file_handle_i_mode:long (task:long, fd:long) %{ /* pure */
 	struct task_struct *p = (struct task_struct *)((long)THIS->task);
-	struct files_struct *files = kread(&p->files);
+	struct files_struct *files;
 	struct file *filp;
+	struct dentry *dentry;
+	struct inode *inode;
 
 	rcu_read_lock();
-	filp = fcheck_files(files, THIS->fd);
-	THIS->__retvalue = kread(&filp->f_dentry->d_inode->i_mode);
-	rcu_read_unlock();
+	if ((files = kread(&p->files)) &&
+	    (filp = fcheck_files(files, THIS->fd)) &&
+	    (dentry = kread(&filp->f_dentry)) &&
+	    (inode = kread(&dentry->d_inode))) {
+		THIS->__retvalue = inode->i_mode;
+	}
 
 	CATCH_DEREF_FAULT();
+	rcu_read_unlock();
 %}
 
 function task_file_handle_majmin_dev:string (task:long, fd:long) %{ /* pure */
 	struct task_struct *p = (struct task_struct *)((long)THIS->task);
-	struct files_struct *files = kread(&p->files);
+	struct files_struct *files;
 	struct file *filp;
+	struct dentry *dentry;
+	struct inode *inode;
+	struct super_block *sb;
 	int dev_nr;
 
 	rcu_read_lock();
-	filp = fcheck_files(files, THIS->fd);
-	dev_nr = kread(&filp->f_dentry->d_inode->i_sb->s_dev);
-	snprintf(THIS->__retvalue, MAXSTRINGLEN,
-		"%d,%d", MAJOR(dev_nr), MINOR(dev_nr));
-	rcu_read_unlock();
+	if ((files = kread(&p->files)) &&
+	    (filp = fcheck_files(files, THIS->fd)) &&
+	    (dentry = kread(&filp->f_dentry)) &&
+	    (inode = kread(&dentry->d_inode)) &&
+	    (sb = kread(&inode->i_sb)) &&
+	    (dev_nr = kread(&sb->s_dev))) {
+		snprintf(THIS->__retvalue, MAXSTRINGLEN,
+			"%d,%d", MAJOR(dev_nr), MINOR(dev_nr));
+	}
 
 	CATCH_DEREF_FAULT();
+	rcu_read_unlock();
 %}
 
 function task_file_handle_majmin_rdev:string (task:long, fd:long) %{ /* pure */
 	struct task_struct *p = (struct task_struct *)((long)THIS->task);
-	struct files_struct *files = kread(&p->files);
+	struct files_struct *files;
 	struct file *filp;
+	struct dentry *dentry;
+	struct inode *inode;
 	int rdev_nr;
 
 	rcu_read_lock();
-	filp = fcheck_files(files, THIS->fd);
-	rdev_nr = kread(&filp->f_dentry->d_inode->i_rdev);
-	snprintf(THIS->__retvalue, MAXSTRINGLEN,
-		"%d,%d", MAJOR(rdev_nr), MINOR(rdev_nr));
-	rcu_read_unlock();
+	if ((files = kread(&p->files)) &&
+	    (filp = fcheck_files(files, THIS->fd)) &&
+	    (dentry = kread(&filp->f_dentry)) &&
+	    (inode = kread(&dentry->d_inode)) &&
+	    (rdev_nr = kread(&inode->i_rdev))) {
+		snprintf(THIS->__retvalue, MAXSTRINGLEN,
+			"%d,%d", MAJOR(rdev_nr), MINOR(rdev_nr));
+	}
 
 	CATCH_DEREF_FAULT();	
+	rcu_read_unlock();
 %}
 
 function task_file_handle_i_node:long (task:long, fd:long) %{ /* pure */
 	struct task_struct *p = (struct task_struct *)((long)THIS->task);
-	struct files_struct *files = kread(&p->files);
+	struct files_struct *files;
 	struct file *filp;
+	struct dentry *dentry;
+	struct inode *inode;
 	
 	rcu_read_lock();
-	filp = fcheck_files(files, THIS->fd);
-	THIS->__retvalue = kread(&filp->f_dentry->d_inode->i_ino);
-	rcu_read_unlock();
+	if ((files = kread(&p->files)) &&
+	    (filp = fcheck_files(files, THIS->fd)) &&
+	    (dentry = kread(&filp->f_dentry)) &&
+	    (inode = kread(&dentry->d_inode))) {
+		THIS->__retvalue = inode->i_ino;
+	}
 
 	CATCH_DEREF_FAULT();
+	rcu_read_unlock();
 %}
 
 function task_file_handle_uid:long (task:long, fd:long) %{ /* pure */
 	struct task_struct *p = (struct task_struct *)((long)THIS->task);
-	struct files_struct *files = kread(&p->files);
+	struct files_struct *files;
 	struct file *filp;
 	
 	rcu_read_lock();
-	filp = fcheck_files(files, THIS->fd);
+	if ((files = kread(&p->files)) &&
+	    (filp = fcheck_files(files, THIS->fd))) {
 #if LINUX_VERSION_CODE >= KERNEL_VERSION(2,6,29)
-	/* git commit d76b0d9b */
-	THIS->__retvalue = kread(&filp->f_cred->fsuid);
+		const struct cred *cred;
+		if ((cred = kread(&filp->f_cred))) {
+			/* git commit d76b0d9b */
+			THIS->__retvalue = cred->fsuid;
+		}
 #else
-	THIS->__retvalue = kread(&filp->f_uid);
+		THIS->__retvalue = kread(&filp->f_uid);
 #endif
-	rcu_read_unlock();
+	}
 
 	CATCH_DEREF_FAULT();
+	rcu_read_unlock();
 %}
 
 function task_file_handle_gid:long (task:long, fd:long) %{ /* pure */
 	struct task_struct *p = (struct task_struct *)((long)THIS->task);
-	struct files_struct *files = kread(&p->files);
+	struct files_struct *files;
 	struct file *filp;
 	
 	rcu_read_lock();
-	filp = fcheck_files(files, THIS->fd);
+	if ((files = kread(&p->files)) &&
+	    (filp = fcheck_files(files, THIS->fd))) {
 #if LINUX_VERSION_CODE >= KERNEL_VERSION(2,6,29)
-	/* git commit d76b0d9b */
-	THIS->__retvalue = kread(&filp->f_cred->fsgid);
+		const struct cred *cred;
+		if ((cred = kread(&filp->f_cred))) {
+			/* git commit d76b0d9b */
+			THIS->__retvalue = cred->fsgid;
+		}
 #else
-	THIS->__retvalue = kread(&filp->f_gid);
+		THIS->__retvalue = kread(&filp->f_gid);
 #endif
-	rcu_read_unlock();
+	}
 
 	CATCH_DEREF_FAULT();
+	rcu_read_unlock();
 %}
 
 function task_file_handle_f_flags:long (task:long, fd:long) %{ /* pure */
 	struct task_struct *p = (struct task_struct *)((long)THIS->task);
-	struct files_struct *files = kread(&p->files);
+	struct files_struct *files;
 	struct file *filp;
 	
 	rcu_read_lock();
-	filp = fcheck_files(files, THIS->fd);
-	THIS->__retvalue = kread(&filp->f_flags);
-	rcu_read_unlock();
+	if ((files = kread(&p->files)) &&
+	    (filp = fcheck_files(files, THIS->fd))) {
+		THIS->__retvalue = kread(&filp->f_flags);
+	}
 
 	CATCH_DEREF_FAULT();
+	rcu_read_unlock();
 %}
 
 function task_file_handle_fd_flags:string (task:long, fd:long) %{ /* pure */
 	struct task_struct *p = (struct task_struct *)((long)THIS->task);
-	struct files_struct *files = kread(&p->files);
+	struct files_struct *files;
 	struct fdtable *fdt;
 	int gcoe;
 	
 	rcu_read_lock();
-	fdt = files_fdtable(files);
-	gcoe = FD_ISSET(THIS->fd, kread(&fdt->close_on_exec));
-	snprintf(THIS->__retvalue, MAXSTRINGLEN,
-		"%s", gcoe ? "FD_CLOEXEC" : "");
-	rcu_read_unlock();
+	if ((files = kread(&p->files)) &&
+	    (fdt = files_fdtable(files))) {
+		gcoe = FD_ISSET(THIS->fd, kread(&fdt->close_on_exec));
+		snprintf(THIS->__retvalue, MAXSTRINGLEN,
+			"%s", gcoe ? "FD_CLOEXEC" : "");
+	}
 
 	CATCH_DEREF_FAULT();
+	rcu_read_unlock();
 %}
 
 function task_file_handle_flock:string (task:long, fd:long) %{ /* pure */
 	struct task_struct *p = (struct task_struct *)((long)THIS->task);
-	struct files_struct *files = kread(&p->files);
+	struct files_struct *files;
 	struct file_lock *flock;
 	int fl_type, fl_pid;
 	struct file *filp;
+	struct dentry *dentry;
+	struct inode *inode;
 
 	rcu_read_lock();
-	filp = fcheck_files(files, THIS->fd);
-	flock = kread(&filp->f_dentry->d_inode->i_flock);
-	fl_type = fl_pid = -1;
-	if (flock) {
-		fl_type = kread(&flock->fl_type);
-		fl_pid = kread(&flock->fl_pid);
-	}
+	if ((files = kread(&p->files)) &&
+	    (filp = fcheck_files(files, THIS->fd)) &&
+	    (dentry = kread(&filp->f_dentry)) &&
+	    (inode = kread(&dentry->d_inode))) {
+		if ((flock = kread(&inode->i_flock))) {
+			fl_type = kread(&flock->fl_type);
+			fl_pid = kread(&flock->fl_pid);
+		} else {
+			fl_type = -1;
+			fl_pid = -1;
+		}
 
-	if (fl_type != -1) /* !flock */
-		snprintf(THIS->__retvalue, MAXSTRINGLEN,
-			"      advisory %s lock set by process %d",
-			fl_type ? "write" : "read", fl_pid);
-	else
-		snprintf(THIS->__retvalue, MAXSTRINGLEN, "NULL");
-	rcu_read_unlock();
+		if (fl_type != -1) { /* !flock */
+			snprintf(THIS->__retvalue, MAXSTRINGLEN,
+				"      advisory %s lock set by process %d",
+				fl_type ? "write" : "read", fl_pid);
+		} else {
+			snprintf(THIS->__retvalue, MAXSTRINGLEN, "NULL");
+		}
+	}
 
 	CATCH_DEREF_FAULT();
+	rcu_read_unlock();
 %}
 
 function task_file_handle_d_path:string (task:long, fd:long) %{ /* pure */
 	struct task_struct *p = (struct task_struct *)((long)THIS->task);
-	struct files_struct *files = kread(&p->files);
-	char *page = (char *)__get_free_page(GFP_KERNEL);
+	struct files_struct *files;
+	char *page = NULL;
 	struct file *filp;
 	struct dentry *dentry;
 	struct vfsmount *vfsmnt;
-
-	/* TODO: handle !page */
-	if (!page)
-		return;
+	char *path = NULL;
 
 	rcu_read_lock();
-	filp = fcheck_files(files, THIS->fd);
-	dentry = kread(&filp->f_dentry);
-	vfsmnt = kread(&filp->f_vfsmnt);
+	if ((files = kread(&p->files)) &&
+	    (page = (char *)__get_free_page(GFP_KERNEL)) &&
+	    (filp = fcheck_files(files, THIS->fd))) {
+
 #if LINUX_VERSION_CODE >= KERNEL_VERSION(2,6,26)
-	/* git commit 9d1bc601 */
-    snprintf(THIS->__retvalue, MAXSTRINGLEN, "%s",
-        d_path(&filp->f_path, page, PAGE_SIZE));
+		/* git commit 9d1bc601 */
+		path = d_path(&filp->f_path, page, PAGE_SIZE);
 #else
-	snprintf(THIS->__retvalue, MAXSTRINGLEN, "%s",
-		d_path(dentry, vfsmnt, page, PAGE_SIZE));
-#endif	
-	free_page((unsigned long)page);
-	rcu_read_unlock();
+		dentry = kread(&filp->f_dentry);
+		vfsmnt = kread(&filp->f_vfsmnt);
 
+		if (dentry && vfsmnt) {
+			path = d_path(dentry, vfsmnt, page, PAGE_SIZE);
+		}
+#endif
+		if (path && !IS_ERR(path)) {
+			snprintf(THIS->__retvalue, MAXSTRINGLEN, "%s", path);
+		}
+	}
 	CATCH_DEREF_FAULT();
+
+	if (page) free_page((unsigned long)page);
+
+	rcu_read_unlock();
 %}
 
 function task_file_handle_socket:long (task:long, fd:long) %{ /* pure */
 	struct task_struct *p = (struct task_struct *)((long)THIS->task);
-	struct files_struct *files = kread(&p->files);
-	struct inode *inode;
+	struct files_struct *files;
 	struct file *filp;
+	struct dentry *dentry;
+	struct inode *inode;
 
 	rcu_read_lock();
-	filp = fcheck_files(files, THIS->fd);
-	if (!filp) {
-		THIS->__retvalue = 0;
-	    rcu_read_unlock();
-		return;
+	if ((files = kread(&p->files)) &&
+	    (filp = fcheck_files(files, THIS->fd)) &&
+	    (dentry = kread(&filp->f_dentry)) &&
+	    (inode = kread(&filp->f_dentry->d_inode))) {
+		if (S_ISSOCK(kread(&inode->i_mode)))
+			THIS->__retvalue = (long)SOCKET_I(inode);
 	}
-	inode = kread(&filp->f_dentry->d_inode);
-	if (S_ISSOCK(kread(&inode->i_mode)))
-		THIS->__retvalue = (long)SOCKET_I(inode);
-	rcu_read_unlock();
 
 	CATCH_DEREF_FAULT();
+	rcu_read_unlock();
 %}
 
 function socket_userlocks:string (sock:long) %{ /* pure */
@@ -391,7 +441,8 @@ function socket_optname:string (sock:long) %{ /* pure */
 
 function socket_family:long (sock:long) %{ /* pure */
 	struct socket *sock = (struct socket *)((long)THIS->sock);
-	THIS->__retvalue = (long)kread(&sock->ops->family);
+	const struct proto_ops *ops = kread(&sock->ops);
+	THIS->__retvalue = (long)kread(ops->family);
 	CATCH_DEREF_FAULT();
 %}
 

--------------1.7.2.1--

Re: checking return values for pfiles.stp

[Josh Stone]

On 10/19/2010 05:09 PM, Erick Tryzelaar wrote:
> Updated with jistone's suggestions.
> 
> On Tue, Oct 19, 2010 at 11:49 AM, Erick Tryzelaar
> <erick.tryzelaar@gmail.com> wrote:
>> Good morning,
>>
>> jistone on #systemtap helped me clean up pfiles.stp to make sure to
>> check return values when running pfiles.stp, which I've attached.
>> Also, would there be any possibility to promote any of pfiles's
>> functionality into a tapset? I find it pretty useful to be able to get
>> a filename for a given fd.
>>
>> Thanks!
>>

Thanks, I've pushed this.  I made a few more tweaks in commit b86b8bd,
if you're interested.

Josh