On 03/12/2014 11:59 AM, Plamen Dimitrov wrote:
It'd be nice to explain in the commit message a bit more about what
changes were made and why. It doesn't need to be anything fancy, just a
short, precise sentence is often enough. Other than that I have only
some PEP8 problems I've spot, see below.
Indeed I see your mailer messed up the patches pretty badly. Did you use
git send-email?
Signed-off-by: Plamen Dimitrov <[email protected]>
---
client/lv_utils.py | 172
+++++++++++++++++++++++++++++++++--------------------
1 file changed, 107 insertions(+), 65 deletions(-)
diff --git a/client/lv_utils.py b/client/lv_utils.py
index 5b01541..26a94b6 100644
--- a/client/lv_utils.py
+++ b/client/lv_utils.py
@@ -55,100 +55,113 @@ def vg_ramdisk(vg_name, ramdisk_vg_size,
ramdisk_filename = os.path.join(vg_ramdisk_dir,
ramdisk_sparse_filename)
- vg_ramdisk_cleanup(ramdisk_filename,
- vg_ramdisk_dir, vg_name, "")
+ vg_ramdisk_cleanup(ramdisk_filename, vg_ramdisk_dir, vg_name)
result = ""
if not os.path.exists(vg_ramdisk_dir):
os.mkdir(vg_ramdisk_dir)
try:
logging.info("Mounting tmpfs")
- result = utils.run("mount -t tmpfs tmpfs " + vg_ramdisk_dir)
+ result = utils.run("mount -t tmpfs tmpfs %s" % vg_ramdisk_dir)
logging.info("Converting and copying /dev/zero")
- cmd = ("dd if=/dev/zero of=" + ramdisk_filename +
- " bs=1M count=1 seek=" + vg_size)
+ cmd = ("dd if=/dev/zero of=%s bs=1M count=1 seek=%s" %
+ (ramdisk_filename, vg_size))
result = utils.run(cmd, verbose=True)
logging.info("Finding free loop device")
result = utils.run("losetup --find", verbose=True)
except error.CmdError, ex:
logging.error(ex)
- vg_ramdisk_cleanup(ramdisk_filename,
- vg_ramdisk_dir, vg_name, "")
+ vg_ramdisk_cleanup(ramdisk_filename, vg_ramdisk_dir, vg_name)
raise ex
loop_device = result.stdout.rstrip()
try:
logging.info("Creating loop device")
- result = utils.run("losetup " + loop_device + " " +
ramdisk_filename)
+ result = utils.run("losetup %s %s" % (loop_device,
ramdisk_filename))
logging.info("Creating physical volume %s", loop_device)
- result = utils.run("pvcreate " + loop_device)
+ result = utils.run("pvcreate %s" % loop_device)
logging.info("Creating volume group %s", vg_name)
- result = utils.run("vgcreate " + vg_name + " " + loop_device)
+ result = utils.run("vgcreate %s %s" % (vg_name, loop_device))
except error.CmdError, ex:
logging.error(ex)
- vg_ramdisk_cleanup(ramdisk_filename, vg_ramdisk_dir, vg_name,
- loop_device)
+ vg_ramdisk_cleanup(ramdisk_filename, vg_ramdisk_dir,
+ vg_name, loop_device)
raise ex
logging.info(result.stdout.rstrip())
-def vg_ramdisk_cleanup(ramdisk_filename, vg_ramdisk_dir,
- vg_name, loop_device):
+def vg_ramdisk_cleanup(ramdisk_filename = None, vg_ramdisk_dir = None,
+ vg_name = None, loop_device = None):
^ According to PEP8, we shouldn't put spaces between parameters and
default assignment, so it'd be like
def vg_ramdisk_cleanup(ramdisk_filename=None, vg_ramdisk_dir=None,
vg_name=None, loop_device=None):
The same comment goes for other places where you added the spaces.
Granted, it's something minor. This is time to shamelessly advertise
inspektor:
https://github.com/lmr/inspektor
Program I started writing (therefore, far from passable) to replace
check patch scripts. You can have it to fix the little style problems with:
$ inspekt style
"""
Inline cleanup function in case of test error.
"""
- result = utils.run("vgremove " + vg_name, ignore_status=True)
- if result.exit_status == 0:
- logging.info(result.stdout.rstrip())
- else:
- logging.debug("%s -> %s", result.command, result.stderr)
-
- result = utils.run("pvremove " + loop_device, ignore_status=True)
- if result.exit_status == 0:
- logging.info(result.stdout.rstrip())
- else:
- logging.debug("%s -> %s", result.command, result.stderr)
-
- for _ in range(10):
- time.sleep(0.1)
- result = utils.run("losetup -d " + loop_device, ignore_status=True)
- if "resource busy" not in result.stderr:
- if result.exit_status != 0:
- logging.debug("%s -> %s", result.command, result.stderr)
- else:
- logging.info("Loop device %s deleted", loop_device)
- break
-
- if os.path.exists(ramdisk_filename):
- os.unlink(ramdisk_filename)
- logging.info("Ramdisk filename %s deleted", ramdisk_filename)
+ if vg_name != None:
Another PEP8 caveat, this comparison above, involving None, is better
written:
if vg_name is not None:
Same goes for other instances.
+ loop_device = re.search("([/\w]+) %s lvm2" % vg_name,
+ utils.run("pvs").stdout)
+ if loop_device != None:
+ loop_device = loop_device.group(1)
+
+ result = utils.run("vgremove %s" % vg_name, ignore_status=True)
+ if result.exit_status == 0:
+ logging.info(result.stdout.rstrip())
+ else:
+ logging.debug("%s -> %s", result.command, result.stderr)
- utils.run("umount " + vg_ramdisk_dir, ignore_status=True)
- if result.exit_status == 0:
- if loop_device != "":
- logging.info("Loop device %s unmounted", loop_device)
- else:
- logging.debug("%s -> %s", result.command, result.stderr)
+ if loop_device != None:
+ result = utils.run("pvremove %s" % loop_device, ignore_status=True)
+ if result.exit_status == 0:
+ logging.info(result.stdout.rstrip())
+ else:
+ logging.debug("%s -> %s", result.command, result.stderr)
+
+ if loop_device in utils.run("losetup --all").stdout:
+ ramdisk_filename = re.search("%s: \[\d+\]:\d+ \(([/\w]+)\)"
% loop_device,
+ utils.run("losetup --all").stdout)
+ if ramdisk_filename != None:
+ ramdisk_filename = ramdisk_filename.group(1)
+
+ for _ in range(10):
+ time.sleep(0.1)
+ result = utils.run("losetup -d %s" % loop_device,
ignore_status=True)
+ if "resource busy" not in result.stderr:
+ if result.exit_status == 0:
+ logging.info("Loop device %s deleted", loop_device)
+ else:
+ logging.debug("%s -> %s", result.command,
result.stderr)
+ break
+
+ if ramdisk_filename != None:
+ if os.path.exists(ramdisk_filename):
+ os.unlink(ramdisk_filename)
+ logging.info("Ramdisk filename %s deleted", ramdisk_filename)
+ vg_ramdisk_dir = os.path.dirname(ramdisk_filename)
+
+ if vg_ramdisk_dir != None:
+ utils.run("umount %s" % vg_ramdisk_dir, ignore_status=True)
+ if result.exit_status == 0:
+ logging.info("Successfully unmounted tmpfs from %s",
vg_ramdisk_dir)
+ else:
+ logging.debug("%s -> %s", result.command, result.stderr)
- if os.path.exists(vg_ramdisk_dir):
- try:
- shutil.rmtree(vg_ramdisk_dir)
- logging.info("Ramdisk directory %s deleted", vg_ramdisk_dir)
- except OSError:
- pass
+ if os.path.exists(vg_ramdisk_dir):
+ try:
+ shutil.rmtree(vg_ramdisk_dir)
+ logging.info("Ramdisk directory %s deleted",
vg_ramdisk_dir)
+ except OSError:
+ pass
def vg_check(vg_name):
"""
Check whether provided volume group exists.
"""
- cmd = "vgdisplay " + vg_name
+ cmd = "vgdisplay %s" % vg_name
try:
utils.run(cmd)
- logging.debug("Provided volume group exists: " + vg_name)
+ logging.debug("Provided volume group exists: %s", vg_name)
return True
except error.CmdError:
return False
@@ -221,11 +234,10 @@ def lv_check(vg_name, lv_name):
result = utils.run(cmd, ignore_status=True)
# unstable approach but currently works
- lvpattern = r"LV Path\s+/dev/" + vg_name + r"/" + lv_name + "\s+"
+ lvpattern = r"LV Path\s+/dev/%s/%s\s+" % (vg_name, lv_name)
match = re.search(lvpattern, result.stdout.rstrip())
if match:
- logging.debug("Provided logical volume exists: /dev/" +
- vg_name + "/" + lv_name)
+ logging.debug("Provided logical volume %s exists in %s",
lv_name, vg_name)
return True
else:
return False
@@ -244,13 +256,13 @@ def lv_remove(vg_name, lv_name):
if not lv_check(vg_name, lv_name):
raise error.TestError("Logical volume could not be found")
- cmd = "lvremove -f " + vg_name + "/" + lv_name
+ cmd = "lvremove -f %s/%s" % (vg_name, lv_name)
result = utils.run(cmd)
logging.info(result.stdout.rstrip())
@error.context_aware
-def lv_create(vg_name, lv_name, lv_size, force_flag=True):
+def lv_create(vg_name, lv_name, lv_size, force_flag = True):
"""
Create a logical volume in a volume group.
@@ -266,7 +278,7 @@ def lv_create(vg_name, lv_name, lv_size,
force_flag=True):
elif lv_check(vg_name, lv_name) and force_flag:
lv_remove(vg_name, lv_name)
- cmd = ("lvcreate --size " + lv_size + " --name " + lv_name + " " +
vg_name)
+ cmd = "lvcreate --size %s --name %s %s" % (lv_size, lv_name, vg_name)
result = utils.run(cmd)
logging.info(result.stdout.rstrip())
@@ -287,8 +299,8 @@ def lv_take_snapshot(vg_name, lv_name,
if not lv_check(vg_name, lv_name):
raise error.TestError("Snapshot's origin could not be found")
- cmd = ("lvcreate --size " + lv_snapshot_size + " --snapshot " +
- " --name " + lv_snapshot_name + " /dev/" + vg_name + "/" +
lv_name)
+ cmd = ("lvcreate --size %s --snapshot --name %s /dev/%s/%s" %
+ (lv_snapshot_size, lv_snapshot_name, vg_name, lv_name))
try:
result = utils.run(cmd)
except error.CmdError, ex:
@@ -328,7 +340,7 @@ def lv_revert(vg_name, lv_name, lv_snapshot_name):
cmd = ("lvconvert --merge /dev/%s/%s" % (vg_name,
lv_snapshot_name))
result = utils.run(cmd)
if ("Merging of snapshot %s will start next activation." %
- lv_snapshot_name) in result.stdout:
+ lv_snapshot_name) in result.stdout:
raise error.TestError("The logical volume %s is still active" %
lv_name)
result = result.stdout.rstrip()
@@ -339,7 +351,7 @@ def lv_revert(vg_name, lv_name, lv_snapshot_name):
if (('Snapshot could not be found' in ex and
re.search(re.escape(lv_snapshot_name + " [active]"),
utils.run("lvdisplay").stdout))
- or ("The logical volume %s is still active" % lv_name)
in ex):
+ or ("The logical volume %s is still active" % lv_name) in ex):
logging.warning(("Logical volume %s is still active! " +
"Attempting to deactivate..."), lv_name)
lv_reactivate(vg_name, lv_name)
@@ -357,7 +369,6 @@ def lv_revert_with_snapshot(vg_name, lv_name,
lv_snapshot_name, lv_snapshot_size):
"""
Perform logical volume merge with snapshot and take a new snapshot.
-
"""
error.context("Reverting to snapshot and taking a new one",
logging.info)
@@ -367,7 +378,7 @@ def lv_revert_with_snapshot(vg_name, lv_name,
@error.context_aware
-def lv_reactivate(vg_name, lv_name, timeout=10):
+def lv_reactivate(vg_name, lv_name, timeout = 10):
"""
In case of unclean shutdowns some of the lvs is still active and
merging
is postponed. Use this function to attempt to deactivate and reactivate
@@ -382,3 +393,34 @@ def lv_reactivate(vg_name, lv_name, timeout=10):
logging.error(("Failed to reactivate %s - please, " +
"nuke the process that uses it first."), lv_name)
raise error.TestError("The logical volume %s is still active" %
lv_name)
+
+
[email protected]_aware
+def lv_mount(vg_name, lv_name, mount_loc, filesystem = False):
+ """
+ Mount a logical volume to a mount location.
+ """
+ error.context("Mounting the logical volume",
+ logging.info)
+
+ try:
+ if filesystem:
+ result = utils.run("mkfs.ext4 /dev/%s/%s" % (vg_name, lv_name))
+ logging.debug(result.stdout.rstrip())
+ result = utils.run("mount /dev/%s/%s %s" % (vg_name, lv_name,
mount_loc))
+ except error.CmdError, ex:
+ logging.warning(ex)
+
+
[email protected]_aware
+def lv_umount(vg_name, lv_name, mount_loc):
+ """
+ Unmount a logical volume from a mount location.
+ """
+ error.context("Unmounting the logical volume",
+ logging.info)
+
+ try:
+ utils.run("umount /dev/%s/%s" % (vg_name, lv_name))
+ except error.CmdError, ex:
+ logging.warning(ex)
_______________________________________________
Autotest-kernel mailing list
[email protected]
https://www.redhat.com/mailman/listinfo/autotest-kernel
_______________________________________________
Autotest-kernel mailing list
[email protected]
https://www.redhat.com/mailman/listinfo/autotest-kernel
