Change in vdsm[master]: Adding remove\disable verbs to vdsm-tool for admin usages

ybronhei at redhat.com ybronhei at redhat.com
Mon Mar 3 13:04:28 UTC 2014


Yaniv Bronhaim has posted comments on this change.

Change subject: Adding remove\disable verbs to vdsm-tool for admin usages
......................................................................


Patch Set 3:

(5 comments)

http://gerrit.ovirt.org/#/c/21772/3//COMMIT_MSG
Commit Message:

Line 3: AuthorDate: 2013-11-27 13:23:56 +0200
Line 4: Commit:     Mooli Tayer <mtayer at redhat.com>
Line 5: CommitDate: 2014-03-02 20:10:47 +0200
Line 6: 
Line 7: Adding remove\disable verbs to vdsm-tool for admin usages
> \ is called 'back slash' and should not be used as far as I know.
interesting ... what i wrote is what my english teacher thought me in highschool. i never doubted about it :/
Line 8: 
Line 9: The spec will be modified separately to use vdsm-tool instead of hard-coded
Line 10: operations
Line 11: 


http://gerrit.ovirt.org/#/c/21772/3/lib/vdsm/tool/configurator.py
File lib/vdsm/tool/configurator.py:

Line 139:         except RuntimeError:
Line 140:             return False
Line 141: 
Line 142:     def removeConf(self):
Line 143:         vdsm_ver = '-4.9.10'
> this is the version of the change not the version of vdsm... like signature
this is how we know in which version of vdsm we introduce new configuration change. only if this number is different we know if to override the configuration with the old or new values. I don't understand why you need it when removing the config.. it does not matter at all. please check first the libvirt_configure.sh, read the code and see what it does. you just need to do the opposite of that..
Line 144:         conf_prefix = '## beginning of configuration section by vdsm' + \
Line 145:                       vdsm_ver
Line 146:         conf_suffix = '## end of configuration section by vdsm' + vdsm_ver
Line 147: 


Line 360:                 c.removeConf()
Line 361:             except:
Line 362:                 failed.append(c.getName)
Line 363:     if failed:
Line 364:         sys.stdout.write(
> yes, other patch if required fixing.
ah? no!
for errors use stderr, for regular output to user such as - start removing... or removed successfully prints use stdout. do it rightly, we don't need to have set of patches here with no reason.
Line 365:             "Could not remove configuration for modules %s\n" % ','.join(failed),
Line 366:         )
Line 367:         raise RuntimeError("Remove configuration failed")
Line 368: 


http://gerrit.ovirt.org/#/c/21772/3/lib/vdsm/tool/passwd.py
File lib/vdsm/tool/passwd.py:

Line 50: def remove_saslpasswd():
Line 51:     """
Line 52:     Remove vdsm password for libvirt connection
Line 53:     """
Line 54:     p = subprocess.Popen(('/usr/sbin/saslpasswd2', '-p', '-a', 'libvirt', '-d', constants.SASL_USERNAME))
> Are you saying /usr/sbin/saslpasswd2 should be in constants.py?
yes
Line 55:     output, err = p.communicate()
Line 56:     if p.returncode != 0:


Line 52:     Remove vdsm password for libvirt connection
Line 53:     """
Line 54:     p = subprocess.Popen(('/usr/sbin/saslpasswd2', '-p', '-a', 'libvirt', '-d', constants.SASL_USERNAME))
Line 55:     output, err = p.communicate()
Line 56:     if p.returncode != 0:
> Yaniv?
utils.execCmd instead of Popen


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie7f2c031436a6d202f856c24d9c9420c8bfdf6df
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Yaniv Bronhaim <ybronhei at redhat.com>
Gerrit-Reviewer: Alon Bar-Lev <alonbl at redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei at redhat.com>
Gerrit-Reviewer: automation at ovirt.org
Gerrit-Reviewer: mooli tayer <mtayer at redhat.com>
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes


More information about the vdsm-patches mailing list