Change in vdsm[master]: BZ#836161 - Pluralize lvm.changelv().

abaron at redhat.com abaron at redhat.com
Mon Oct 15 09:40:55 UTC 2012


Ayal Baron has posted comments on this change.

Change subject: BZ#836161 - Pluralize lvm.changelv().
......................................................................


Patch Set 3: I would prefer that you didn't submit this

(6 inline comments)

....................................................
File vdsm/storage/blockVolume.py
Line 646: 
Line 647:     lvNames = tuple(vol.volUUID for vol in volumes)
Line 648:     #Assert volumes are writable. (Don't do this at home.)
Line 649:     try:
Line 650:         lvm.changelv(sdUUID, lvNames, ("--permission", "rw"))
why not move this into a variable?
Line 651:     except se.StorageException, e:
Line 652:         # Hope this only means that some volumes were already writable.
Line 653:         pass
Line 654: 


....................................................
File vdsm/storage/lvm.py
Line 729:     """
Line 730:     Change multiple attributes on multiple LVs.
Line 731: 
Line 732:     vg: VG name
Line 733:     lvs: a single LV name or LV names iterable.
s/a single.*/iterable list of lvs/
Line 734:     attrs: a (attr, value) tuple or a tuple of (attr, value) tuples.
Line 735: 
Line 736:     Note:
Line 737:     You may activate an activated LV without error


Line 730:     Change multiple attributes on multiple LVs.
Line 731: 
Line 732:     vg: VG name
Line 733:     lvs: a single LV name or LV names iterable.
Line 734:     attrs: a (attr, value) tuple or a tuple of (attr, value) tuples.
s/a (.*/an iterable of (attr, value) pairs), e.g. (('--available', 'y'), ('--permission', 'rw'))/
Line 735: 
Line 736:     Note:
Line 737:     You may activate an activated LV without error
Line 738:     but lvchange returns an error (RC=5) when activating rw if already rw


Line 738:     but lvchange returns an error (RC=5) when activating rw if already rw
Line 739:     """
Line 740: 
Line 741:     lvs = _normalizeargs(lvs)
Line 742:     # If it fails or not we (may be) change the lv,
s/may be/maybe/
Line 743:     # so we invalidate cache to reload these volumes on first occasion
Line 744:     lvnames = tuple("%s/%s" % (vg, lv) for lv in lvs)
Line 745:     cmd = ["lvchange"]
Line 746:     cmd.extend(LVM_NOBACKUP)


Line 744:     lvnames = tuple("%s/%s" % (vg, lv) for lv in lvs)
Line 745:     cmd = ["lvchange"]
Line 746:     cmd.extend(LVM_NOBACKUP)
Line 747:     for attr in attrs:
Line 748:         if type(attr) == type(''):
s/.*/isinstance(attr, types.StringTypes)/
Line 749:             cmd.append(attr)
Line 750:         else:
Line 751:             cmd.extend(attr)
Line 752:     cmd.extend(lvnames)


Line 745:     cmd = ["lvchange"]
Line 746:     cmd.extend(LVM_NOBACKUP)
Line 747:     for attr in attrs:
Line 748:         if type(attr) == type(''):
Line 749:             cmd.append(attr)
you could do this in a single 'if' on attrs
Line 750:         else:
Line 751:             cmd.extend(attr)
Line 752:     cmd.extend(lvnames)
Line 753:     rc, out, err = _lvminfo.cmd(tuple(cmd))


--
To view, visit http://gerrit.ovirt.org/8289
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I22c4ef83f23c0004a5a1e6a8a7e55d13b56c4f50
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Eduardo <ewarszaw at redhat.com>
Gerrit-Reviewer: Ayal Baron <abaron at redhat.com>
Gerrit-Reviewer: Eduardo <ewarszaw at redhat.com>
Gerrit-Reviewer: Yeela Kaplan <ykaplan at redhat.com>


More information about the vdsm-patches mailing list