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

[qiaonuohan]

At 2012-3-13 11:45, qiaonuohan wrote:
> 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.
>

The attachment is the modified patch.

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


-- 
--
Regards
Qiao Nuohan
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: vm.patch
URL: <http://listman.redhat.com/archives/crash-utility/attachments/20120313/288a5eb5/attachment.ksh>