Re: [Crash-utility] [PATCH] fix command vm/files -d/mount on new kernel

At 2012-3-13 3:27, Dave Anderson wrote: > > > ----- Original Message ----- >> At 2012-3-10 5:50, Dave Anderson wrote: >>> >>> It's failing in its call to get_pathname(). I haven't had a chance to >> >> You are right. I forgot to check every call to get_pathname. The new >> patch has fixed the problem of swap. > > Although the patch appears to work in the simple cases that I have > tested, > I have a few issues with it. > > First, the get_pathname() function prototype is -- and has always been > -- this: > > void get_pathname(ulong dentry, char *pathname, int length, int full, > ulong vfsmnt) > > where the "vfsmnt" argument has always been a struct vfsmount pointer. > > With your patch, prior to making get_pathname() calls, you typically > make a VALID_STRUCT(mount) call first, and if true, you modify the > vfsmnt argument to be a struct mount pointer. > > It would make more sense maintenance-wise, and would simplify the patch, > if the get_pathname() "vfsmnt" argument would continue to be a struct > vfsmount pointer. And at the top of get_pathname(), you could make the > adjustments for when it is embedded inside a struct mount. > > For example, taking these patches to open_files_dump() and file_dump(): > > @@ -2226,12 +2299,16 @@ open_files_dump(ulong task, int flags, struct > reference *ref) > if (VALID_MEMBER(fs_struct_rootmnt)) { > vfsmnt = ULONG(fs_struct_buf + > OFFSET(fs_struct_rootmnt)); > + if (VALID_STRUCT(mount)) > + vfsmnt = vfsmnt - OFFSET(mount_mnt); > get_pathname(root_dentry, root_pathname, > BUFSIZE, 1, vfsmnt); > } else if (use_path) { > vfsmnt = ULONG(fs_struct_buf + > OFFSET(fs_struct_root) + > OFFSET(path_mnt)); > + if (VALID_STRUCT(mount)) > + vfsmnt = vfsmnt - OFFSET(mount_mnt); > get_pathname(root_dentry, root_pathname, > BUFSIZE, 1, vfsmnt); > } else { > @@ -2250,12 +2327,16 @@ open_files_dump(ulong task, int flags, struct > reference *ref) > if (VALID_MEMBER(fs_struct_pwdmnt)) { > vfsmnt = ULONG(fs_struct_buf + > OFFSET(fs_struct_pwdmnt)); > + if (VALID_STRUCT(mount)) > + vfsmnt = vfsmnt - OFFSET(mount_mnt); > get_pathname(pwd_dentry, pwd_pathname, > BUFSIZE, 1, vfsmnt); > } else if (use_path) { > vfsmnt = ULONG(fs_struct_buf + > OFFSET(fs_struct_pwd) + > OFFSET(path_mnt)); > + if (VALID_STRUCT(mount)) > + vfsmnt = vfsmnt - OFFSET(mount_mnt); > get_pathname(pwd_dentry, pwd_pathname, > BUFSIZE, 1, vfsmnt); > @@ -2686,7 +2767,12 @@ file_dump(ulong file, ulong dentry, ulong > inode, int fd, int flags) > if (flags& DUMP_FULL_NAME) { > if (VALID_MEMBER(file_f_vfsmnt)) { > vfsmnt = get_root_vfsmount(file_buf); > - get_pathname(dentry, pathname, BUFSIZE, 1, vfsmnt); > + if (VALID_STRUCT(mount)) > + get_pathname(dentry, pathname, BUFSIZE, 1, > + vfsmnt - OFFSET(mount_mnt)); > + else > + get_pathname(dentry, pathname, BUFSIZE, 1, > + vfsmnt); > if (STRNEQ(pathname, "/pts/")&& > STREQ(vfsmount_devname(vfsmnt, buf1, BUFSIZE), > "devpts")) > > If the vfsmount-to-mount calculation were always done in > get_pathname() itself, > then the patches above would be unnecessary. The same is true for a > few other > parts of the patch. > > Did you consider doing it that way? > > And consider the following patched version of display_dentry_info() > below. > The two get_pathname() calls below would generate fatal errors on older > kernels because OFFSET(mount_mnt) will be invalid: > > if (!found&& symbol_exists("pipe_mnt")) { > get_symbol_data("pipe_mnt", sizeof(long),&vfs); > if (VALID_STRUCT(mount)) > readmem(vfs - OFFSET(mount_mnt), KVADDR, mount_buf, SIZE(mount), > "mount buffer", FAULT_ON_ERROR); > else > readmem(vfs, KVADDR, vfsmount_buf, SIZE(vfsmount), > "vfsmount buffer", FAULT_ON_ERROR); > sb = ULONG(vfsmount_buf + OFFSET(vfsmount_mnt_sb)); > if (superblock&& (sb == superblock)) { > get_pathname(dentry, pathname, BUFSIZE, 1, > vfs - OFFSET(mount_mnt)); > found = TRUE; > } > } > if (!found&& symbol_exists("sock_mnt")) { > get_symbol_data("sock_mnt", sizeof(long),&vfs); > if (VALID_STRUCT(mount)) > readmem(vfs - OFFSET(mount_mnt), KVADDR, mount_buf, SIZE(mount), > "mount buffer", FAULT_ON_ERROR); > else > readmem(vfs, KVADDR, vfsmount_buf, SIZE(vfsmount), > "vfsmount buffer", FAULT_ON_ERROR); > sb = ULONG(vfsmount_buf + OFFSET(vfsmount_mnt_sb)); > if (superblock&& (sb == superblock)) { > get_pathname(dentry, pathname, BUFSIZE, 1, > vfs - OFFSET(mount_mnt)); > found = TRUE; > } > } > > If we can continue to have get_pathname() receive a vfsmount pointer, > it reduces the chance of introducing new bugs like the above, and future > maintenance will be easier. It does reduce complication of the code. Still, places needing the judgement of struct mount's existing remain. I will modify the patch, and post later. About the function, OFFSET_OPTION(), thanks for your advice.

> > Comments? > > Dave > > -- > Crash-utility mailing list > Crash-utility(a)redhat.com > https://www.redhat.com/mailman/listinfo/crash-utility > >