[Crash-utility] [PATCH] fix command vm/files -d/mount on new kernel
qiaonuohan
qiaonuohan at cn.fujitsu.com
Tue Mar 13 03:45:02 UTC 2012
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 at redhat.com
> https://www.redhat.com/mailman/listinfo/crash-utility
>
>
--
--
Regards
Qiao Nuohan
More information about the Crash-utility
mailing list