On Fri, Jul 1, 2011 at 3:55 AM, Shrirang Phansalkar <[email protected]> wrote: > Hi, > I am sending the patch for adding new functionality in fsdev_disks.py , so > that it would return unpartitioned as well as unmounted disk as well. > Currently I am not having imap setup in company so won't be able to send > patch via command line.
Hi Shirang, I went through your change. My biggest concern is, even though you have refactored one of the procedures with get_mount_points, the functions get_disk_list and get_full_disk_list still have a lot of common, redundant code on them. It looks better to just add an additional parameter to get_disk_list (say, list_all_devices) which defaults to False. Depending on the value of this parameter, behave differently (list_all_devices=True, every disk device even the unmounted, unpartitioned, list_all_devices=False, only the partitioned disks with a mount point). Some small comments in the patch body. > > From c5cf37657dc4ea8b26dbaee67931cbe4a15f9248 Mon Sep 17 00:00:00 2001 > From: Shrirang Phansalkar <[email protected]> > Date: Thu, 30 Jun 2011 17:41:24 +0530 > Subject: [PATCH] fsdev_disks.py : add function to return unpartitioned device > list. > > Added get_full_disk_list() which will return the list of un-partitioned and > partitioned disk, > as well as unmounted disk as well.For that purpose added get_mount_points() > which was, > common code part between get_full_disk_list() and get_disk_list() > > Signed-off-by: Shrirang Phansalkar <[email protected]> > --- > client/bin/fsdev_disks.py | 122 > ++++++++++++++++++++++++++++++++------------- > 1 files changed, 87 insertions(+), 35 deletions(-) > > diff --git a/client/bin/fsdev_disks.py b/client/bin/fsdev_disks.py > index a4cb390..4a2238b 100644 > --- a/client/bin/fsdev_disks.py > +++ b/client/bin/fsdev_disks.py > @@ -67,42 +67,11 @@ def get_disk_list(std_mounts_only=True): > fstype = '' > fsopts = '' > fsmkfs = '?' > - > # Prepare the full device path for matching > chkdev = '/dev/' + partname > > - # If the partition is mounted, we'll record the mount point > - mountpt = None > - > - for mln in mounts: > - > - splt = mln.split() > - > - # Typical 'mount' output line looks like this (indices > - # for the split() result shown below): > - # > - # <device> on <mount_point> type <fstp> <options> > - # 0 1 2 3 4 5 > - > - if splt[0] == chkdev: > - > - # Make sure the mount point looks reasonable > - mountpt = fd_mgr.check_mount_point(partname, splt[2]) > - if not mountpt: > - mstat = -1 > - break > - > - # Grab the file system type and mount options > - fstype = splt[4] > - fsopts = splt[5] > - > - # Check for something other than a r/w mount > - if fsopts[:3] != '(rw': > - mstat = -1 > - break > - > - # The drive is mounted at the 'normal' mount point > - mstat = 1 > + (mstat, mountpt, fsopts, fstype, partname) = \ > +get_mount_points(mounts, chkdev, mstat, partname, fsopts, fstype) > > # Does the caller only want to allow 'standard' mount points? > if std_mounts_only and mstat < 0: > @@ -110,10 +79,9 @@ def get_disk_list(std_mounts_only=True): > > # Was this partition mounted at all? > if not mountpt: > - # Ask the client where we should mount this partition > mountpt = fd_mgr.check_mount_point(partname, None) > + # Ask the client where we should mount this partition > if not mountpt: > - # Client doesn't know where to mount partition - ignore it > continue > > # Looks like we have a valid disk drive, add it to the list > @@ -128,6 +96,90 @@ def get_disk_list(std_mounts_only=True): > return hd_list > > > +def get_mount_points(mounts, chkdev, mstat, partname, fsopts, fstype, > get_all_disks=False): ^ Here, the param get_all_disks was introduced by no use for it in the function body. > + mountpt = None > + for mln in mounts: > + splt = mln.split() > + > + # Typical 'mount' output line looks like this (indices > + # for the split() result shown below): > + # > + # <device> on <mount_point> type <fstp> <options> > + # 0 1 2 3 4 5 > + > + if splt[0].strip() == chkdev.strip(): > + > + # Make sure the mount point looks reasonable > + mountpt = fd_mgr.check_mount_point(partname, splt[2]) > + if not mountpt: > + mstat = -1 > + break > + > + # Grab the file syystem type and mount options > + fstype = splt[4] > + fsopts = splt[5] > + > + # Check for something other than a r/w mount > + if fsopts[:3] != '(rw': > + mstat = -1 > + break > + # The drive is mounted at the 'normal' mount point > + mstat = 1 > + > + return mstat, mountpt, fstype, fsopts, partname > + > + > +def get_full_disk_list(std_mounts_only=True): > + # Get hold of the currently mounted file systems > + mounts = utils.system_output('mount').splitlines() > + > + # Grab all the interesting disk partition names from /proc/partitions, > + # and build up the table of drives present in the system. > + hd_list = [] > + hd_regexp = re.compile("([hs]d[a-z]+3)$") ^ I was thinking we could add v to [hs] part, as we could be running this code on a virtualized environment.... nipticking though :) > + partfile = open(_DISKPART_FILE) > + for partline in partfile : > + parts = partline.strip().split() > + if len(parts) != 4 or partline.startswith('major'): > + continue > + > + # Get hold of the partition name > + partname = parts[3] > + > + # Process any site-specific filter on the partition name > + if not fd_mgr.use_partition(partname): > + continue > + > + # We need to know the IDE/SATA/... device name for setting tunables > + tunepath = fd_mgr.map_drive_name(partname) > + > + # Check whether the device is mounted (and how) > + mstat = 0 > + fstype = '' > + fsopts = '' > + fsmkfs = '?' > + > + # Prepare the full device path for matching > + chkdev = '/dev/' + partname > + > + (mstat, mountpt, fsopts, fstype, partname) = \ > +get_mount_points(mounts, chkdev, mstat, partname, fsopts, fstype, > get_all_disks=True) > + > + # Does the caller only want to allow 'standard' mount points? > + if std_mounts_only and mstat < 0: > + continue > + > + hd_list.append({ 'device' : partname, > + 'mountpt' : mountpt, > + 'tunable' : tunepath, > + 'fs_type' : fstype, > + 'fs_opts' : fsopts, > + 'fs-mkfs' : fsmkfs, > + 'mounted' : mstat }) > + return hd_list > + > + > def mkfs_all_disks(job, disk_list, fs_type, fs_makeopt, fs_mnt_opt): > """ > Prepare all the drives in 'disk_list' for testing. For each disk this > means > -- > 1.7.4.4 > > > I am attaching the patch also with this mail, please go through it. > > Thanks and regards, > Shrirang > > > > PROPRIETARY-CONFIDENTIAL INFORMATION INCLUDED > > This electronic transmission, and any documents attached hereto, may contain > confidential, proprietary and/or legally privileged information. The > information is intended only for use by the recipient named above. If you > received this electronic message in error, please notify the sender and > delete the electronic message. Any disclosure, copying, distribution, or use > of the contents of information received in error is strictly prohibited, and > violators will be pursued legally. > > _______________________________________________ > Autotest mailing list > [email protected] > http://test.kernel.org/cgi-bin/mailman/listinfo/autotest > > -- Lucas _______________________________________________ Autotest mailing list [email protected] http://test.kernel.org/cgi-bin/mailman/listinfo/autotest
