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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/crash-utility




--
--
Regards
Qiao Nuohan

--
Crash-utility mailing list
Crash-utility@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/crash-utility


[Index of Archives]     [Fedora Development]     [Fedora Desktop]     [Fedora SELinux]     [Yosemite News]     [KDE Users]     [Fedora Tools]

 

Powered by Linux