12:33 a.m.
Wenyi Gao has uploaded a new change for review.
Change subject: Add some base functions to vdsm-tool
......................................................................
Add some base functions to vdsm-tool
Add some base functions to vdsm-tool in order that these functions
can be avalilale for subsequent patches for moving vdsmd.init to
vdsm-tool
Change-Id: Id1be737adce28e9ca532aa454e7f0a060947589e
Signed-off-by: Wenyi Gao <wenyi(a)linux.vnet.ibm.com>
---
M vdsm-tool/Makefile.am
A vdsm-tool/base.py
M vdsm.spec.in
3 files changed, 92 insertions(+), 1 deletion(-)
git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/64/5264/1
--
To view, visit http://gerrit.ovirt.org/5264
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: Id1be737adce28e9ca532aa454e7f0a060947589e
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Wenyi Gao <wenyi(a)linux.vnet.ibm.com>
3:50 a.m.
Wenyi Gao has abandoned this change.
Change subject: Add some base functions to vdsm-tool
......................................................................
Patch Set 10: Abandoned
Replace it with another patch
--
To view, visit http://gerrit.ovirt.org/5264
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: abandon
Gerrit-Change-Id: Id1be737adce28e9ca532aa454e7f0a060947589e
Gerrit-PatchSet: 10
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Wenyi Gao <wenyi(a)linux.vnet.ibm.com>
Gerrit-Reviewer: Adam Litke <agl(a)us.ibm.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Douglas Schilling Landgraf <dougsland(a)redhat.com>
Gerrit-Reviewer: Ewoud Kohl van Wijngaarden <ewoud(a)kohlvanwijngaarden.nl>
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com>
Gerrit-Reviewer: Wenyi Gao <wenyi(a)linux.vnet.ibm.com>
12:48 a.m.
Wenyi Gao has posted comments on this change.
Change subject: Add some base functions to vdsm-tool
......................................................................
Patch Set 8: (1 inline comment)
....................................................
File vdsm-tool/base.py
Line 67: rc = subprocess.call([cmd], shell=True)
It should be subprocess.call([srccmd + tmpcmd], shell=True)
--
To view, visit http://gerrit.ovirt.org/5264
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Id1be737adce28e9ca532aa454e7f0a060947589e
Gerrit-PatchSet: 8
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Wenyi Gao <wenyi(a)linux.vnet.ibm.com>
Gerrit-Reviewer: Adam Litke <agl(a)us.ibm.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Douglas Schilling Landgraf <dougsland(a)redhat.com>
Gerrit-Reviewer: Ewoud Kohl van Wijngaarden <ewoud(a)kohlvanwijngaarden.nl>
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com>
Gerrit-Reviewer: Wenyi Gao <wenyi(a)linux.vnet.ibm.com>
11 p.m.
Wenyi Gao has posted comments on this change.
Change subject: Add some base functions to vdsm-tool
......................................................................
Patch Set 8: (2 inline comments)
....................................................
File vdsm-tool/base.py
Line 47: def call_init_function(args, kwargs={}):
Looks better.
Line 67: rc = subprocess.call([cmd], shell=True)
Very nice. Learned very much from you, thanks.
--
To view, visit http://gerrit.ovirt.org/5264
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Id1be737adce28e9ca532aa454e7f0a060947589e
Gerrit-PatchSet: 8
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Wenyi Gao <wenyi(a)linux.vnet.ibm.com>
Gerrit-Reviewer: Adam Litke <agl(a)us.ibm.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Douglas Schilling Landgraf <dougsland(a)redhat.com>
Gerrit-Reviewer: Ewoud Kohl van Wijngaarden <ewoud(a)kohlvanwijngaarden.nl>
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com>
Gerrit-Reviewer: Wenyi Gao <wenyi(a)linux.vnet.ibm.com>
3:58 a.m.
Ewoud Kohl van Wijngaarden has posted comments on this change.
Change subject: Add some base functions to vdsm-tool
......................................................................
Patch Set 8: I would prefer that you didn't submit this
(2 inline comments)
....................................................
File vdsm-tool/base.py
Line 47: def call_init_function(args, kwargs={}):
I still think (function, *args, **kwargs) would be better. function is required, but the
rest isn't.
Line 67: rc = subprocess.call([cmd], shell=True)
Maybe I'm a perfectionist, but how about:
subprocess.call([srccmd, tmpcmd], shell=True)
Then you can rename tmpcmd to cmd.
--
To view, visit http://gerrit.ovirt.org/5264
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Id1be737adce28e9ca532aa454e7f0a060947589e
Gerrit-PatchSet: 8
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Wenyi Gao <wenyi(a)linux.vnet.ibm.com>
Gerrit-Reviewer: Adam Litke <agl(a)us.ibm.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Douglas Schilling Landgraf <dougsland(a)redhat.com>
Gerrit-Reviewer: Ewoud Kohl van Wijngaarden <ewoud(a)kohlvanwijngaarden.nl>
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com>
Gerrit-Reviewer: Wenyi Gao <wenyi(a)linux.vnet.ibm.com>
8:45 p.m.
Wenyi Gao has posted comments on this change.
Change subject: Add some base functions to vdsm-tool
......................................................................
Patch Set 7: (2 inline comments)
....................................................
File vdsm-tool/base.py
Line 47: def call_init_function(*args, **kwargs):
Yes, good point. I will have it as an explicit argument. Thanks.
Line 59: cmd2 += ' '.join(args)
Yes, you are right. Thanks.
--
To view, visit http://gerrit.ovirt.org/5264
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Id1be737adce28e9ca532aa454e7f0a060947589e
Gerrit-PatchSet: 7
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Wenyi Gao <wenyi(a)linux.vnet.ibm.com>
Gerrit-Reviewer: Adam Litke <agl(a)us.ibm.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Douglas Schilling Landgraf <dougsland(a)redhat.com>
Gerrit-Reviewer: Ewoud Kohl van Wijngaarden <ewoud(a)kohlvanwijngaarden.nl>
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com>
Gerrit-Reviewer: Wenyi Gao <wenyi(a)linux.vnet.ibm.com>
9:11 a.m.
Ewoud Kohl van Wijngaarden has posted comments on this change.
Change subject: Add some base functions to vdsm-tool
......................................................................
Patch Set 7: (2 inline comments)
....................................................
File vdsm-tool/base.py
Line 47: def call_init_function(*args, **kwargs):
In that case you could also use:
if not args:
But function is a required argument that can be handled at python level. Absence implies a
bug that will now only show up in logs.
Line 59: cmd2 += ' '.join(args)
How is it optional?
kwargs = {'a': 'b'}
args = ('x', 'y')
This will result in:
cmd2 = 'a=bx y'
While you would expect:
cmd2 = 'a=b x y'
--
To view, visit http://gerrit.ovirt.org/5264
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Id1be737adce28e9ca532aa454e7f0a060947589e
Gerrit-PatchSet: 7
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Wenyi Gao <wenyi(a)linux.vnet.ibm.com>
Gerrit-Reviewer: Adam Litke <agl(a)us.ibm.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Douglas Schilling Landgraf <dougsland(a)redhat.com>
Gerrit-Reviewer: Ewoud Kohl van Wijngaarden <ewoud(a)kohlvanwijngaarden.nl>
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com>
Gerrit-Reviewer: Wenyi Gao <wenyi(a)linux.vnet.ibm.com>
9:01 a.m.
Wenyi Gao has posted comments on this change.
Change subject: Add some base functions to vdsm-tool
......................................................................
Patch Set 7: (4 inline comments)
....................................................
File vdsm-tool/base.py
Line 47: def call_init_function(*args, **kwargs):
We can also add a test:
if len(args) == 0:
logging.error("No function call")
return rc
Line 52: cmd1 = 'SYSTEMCTL_SKIP_REDIRECT=true source /etc/init.d/functions;'
Looks reasonable.
Line 59: cmd2 += ' '.join(args)
Actually the ' ' is optional.
Line 61: if ';' in cmd2:
Yes. && and || should be added.
--
To view, visit http://gerrit.ovirt.org/5264
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Id1be737adce28e9ca532aa454e7f0a060947589e
Gerrit-PatchSet: 7
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Wenyi Gao <wenyi(a)linux.vnet.ibm.com>
Gerrit-Reviewer: Adam Litke <agl(a)us.ibm.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Douglas Schilling Landgraf <dougsland(a)redhat.com>
Gerrit-Reviewer: Ewoud Kohl van Wijngaarden <ewoud(a)kohlvanwijngaarden.nl>
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com>
Gerrit-Reviewer: Wenyi Gao <wenyi(a)linux.vnet.ibm.com>
7:07 a.m.
Ewoud Kohl van Wijngaarden has posted comments on this change.
Change subject: Add some base functions to vdsm-tool
......................................................................
Patch Set 7: I would prefer that you didn't submit this
(4 inline comments)
I think it's a good start, but needs some work. Comments inline.
....................................................
File vdsm-tool/base.py
Line 47: def call_init_function(*args, **kwargs):
I think it would be better to have the function as an explicit argument. Currently you can
call the function without args and an error because args[0] can't be found.
Line 52: cmd1 = 'SYSTEMCTL_SKIP_REDIRECT=true source /etc/init.d/functions;'
How about source_cmd instead of cmd1?
Line 59: cmd2 += ' '.join(args)
I think this should be:
cmd2 += ' ' + ' '.join(args)
Line 61: if ';' in cmd2:
Nice. Maybe add && and || as well?
--
To view, visit http://gerrit.ovirt.org/5264
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Id1be737adce28e9ca532aa454e7f0a060947589e
Gerrit-PatchSet: 7
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Wenyi Gao <wenyi(a)linux.vnet.ibm.com>
Gerrit-Reviewer: Adam Litke <agl(a)us.ibm.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Douglas Schilling Landgraf <dougsland(a)redhat.com>
Gerrit-Reviewer: Ewoud Kohl van Wijngaarden <ewoud(a)kohlvanwijngaarden.nl>
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com>
Gerrit-Reviewer: Wenyi Gao <wenyi(a)linux.vnet.ibm.com>
8:52 p.m.
Wenyi Gao has posted comments on this change.
Change subject: Add some base functions to vdsm-tool
......................................................................
Patch Set 6: (1 inline comment)
....................................................
File vdsm-tool/base.py
Line 52: rc = subprocess.call([cmd], shell=True)
Ewoud, good suggestion. it's very helpful. Thank you.
--
To view, visit http://gerrit.ovirt.org/5264
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Id1be737adce28e9ca532aa454e7f0a060947589e
Gerrit-PatchSet: 6
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Wenyi Gao <wenyi(a)linux.vnet.ibm.com>
Gerrit-Reviewer: Adam Litke <agl(a)us.ibm.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Douglas Schilling Landgraf <dougsland(a)redhat.com>
Gerrit-Reviewer: Ewoud Kohl van Wijngaarden <ewoud(a)kohlvanwijngaarden.nl>
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com>
Gerrit-Reviewer: Wenyi Gao <wenyi(a)linux.vnet.ibm.com>
5:41 a.m.
Ewoud Kohl van Wijngaarden has posted comments on this change.
Change subject: Add some base functions to vdsm-tool
......................................................................
Patch Set 6: (1 inline comment)
....................................................
File vdsm-tool/base.py
Line 52: rc = subprocess.call([cmd], shell=True)
Since we only need 2 functions (success and failure) you could work with a fixed list of
commands you accept:
def call_init_function(function):
allowed = ('success', 'failure')
if function in allowed:
cmd = 'SYSTEMCTL_SKIP_REDIRECT=true source /etc/init.d/functions; %s' %
function
The alternative would be to rewrite the shell function into python but I don't know
how much the functions change between different releases.
--
To view, visit http://gerrit.ovirt.org/5264
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Id1be737adce28e9ca532aa454e7f0a060947589e
Gerrit-PatchSet: 6
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Wenyi Gao <wenyi(a)linux.vnet.ibm.com>
Gerrit-Reviewer: Adam Litke <agl(a)us.ibm.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Douglas Schilling Landgraf <dougsland(a)redhat.com>
Gerrit-Reviewer: Ewoud Kohl van Wijngaarden <ewoud(a)kohlvanwijngaarden.nl>
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com>
Gerrit-Reviewer: Wenyi Gao <wenyi(a)linux.vnet.ibm.com>
3:59 a.m.
Wenyi Gao has posted comments on this change.
Change subject: Add some base functions to vdsm-tool
......................................................................
Patch Set 6: (2 inline comments)
This is the base patch for my sequent patches for moving vdsmd.init script to vdsm-tool.
As you know, the vdsmd.init shell script call some shell commands and functions in
/etc/init.d/functions, so I add two warrper function "exec_command" and
"call_init_function" to call them.
The vdsmd.init script is a init script, so the "print_msg" is used to print some
messages followed by "[ OK ] or [Failed]", as do other init scripts.
The vdsmd.init shell script need to read some configuration from configuration file, so
the function "get_config_item" is added to get them.
....................................................
File vdsm-tool/base.py
Line 26: def exec_command(argv):
The base.py is installed in the directory "site-packages/vdsm/tool". While the
implement of "execCmd" is in /usr/share/vdsm/storage/misc", which
can't be accessed by base.py.
Line 52: rc = subprocess.call([cmd], shell=True)
Yes, "shell=True" is dangerous, But I can't find a better way to call two
shell commands in sequence.
--
To view, visit http://gerrit.ovirt.org/5264
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Id1be737adce28e9ca532aa454e7f0a060947589e
Gerrit-PatchSet: 6
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Wenyi Gao <wenyi(a)linux.vnet.ibm.com>
Gerrit-Reviewer: Adam Litke <agl(a)us.ibm.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Douglas Schilling Landgraf <dougsland(a)redhat.com>
Gerrit-Reviewer: Ewoud Kohl van Wijngaarden <ewoud(a)kohlvanwijngaarden.nl>
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com>
Gerrit-Reviewer: Wenyi Gao <wenyi(a)linux.vnet.ibm.com>
1:42 a.m.
Dan Kenigsberg has posted comments on this change.
Change subject: Add some base functions to vdsm-tool
......................................................................
Patch Set 6: I would prefer that you didn't submit this
(2 inline comments)
I am mostly missing the use case for these utility function. Without it, I cannot judge if
they location is reasonable
....................................................
File vdsm-tool/base.py
Line 26: def exec_command(argv):
why is this any better than vdsm.utils.execCmd ?
Line 52: rc = subprocess.call([cmd], shell=True)
shell=True is very dangerous (think of value=';rm -fr /').
Please avoid it.
--
To view, visit http://gerrit.ovirt.org/5264
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Id1be737adce28e9ca532aa454e7f0a060947589e
Gerrit-PatchSet: 6
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Wenyi Gao <wenyi(a)linux.vnet.ibm.com>
Gerrit-Reviewer: Adam Litke <agl(a)us.ibm.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Douglas Schilling Landgraf <dougsland(a)redhat.com>
Gerrit-Reviewer: Ewoud Kohl van Wijngaarden <ewoud(a)kohlvanwijngaarden.nl>
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com>
Gerrit-Reviewer: Wenyi Gao <wenyi(a)linux.vnet.ibm.com>
7:25 a.m.
Douglas Schilling Landgraf has posted comments on this change.
Change subject: Add some base functions to vdsm-tool
......................................................................
Patch Set 5: Looks good to me, but someone else must approve
--
To view, visit http://gerrit.ovirt.org/5264
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Id1be737adce28e9ca532aa454e7f0a060947589e
Gerrit-PatchSet: 5
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Wenyi Gao <wenyi(a)linux.vnet.ibm.com>
Gerrit-Reviewer: Adam Litke <agl(a)us.ibm.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Douglas Schilling Landgraf <dougsland(a)redhat.com>
Gerrit-Reviewer: Ewoud Kohl van Wijngaarden <ewoud(a)kohlvanwijngaarden.nl>
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com>
Gerrit-Reviewer: Wenyi Gao <wenyi(a)linux.vnet.ibm.com>
10:33 a.m.
Ewoud Kohl van Wijngaarden has posted comments on this change.
Change subject: Add some base functions to vdsm-tool
......................................................................
Patch Set 5: Looks good to me, but someone else must approve
--
To view, visit http://gerrit.ovirt.org/5264
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Id1be737adce28e9ca532aa454e7f0a060947589e
Gerrit-PatchSet: 5
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Wenyi Gao <wenyi(a)linux.vnet.ibm.com>
Gerrit-Reviewer: Adam Litke <agl(a)us.ibm.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Douglas Schilling Landgraf <dougsland(a)redhat.com>
Gerrit-Reviewer: Ewoud Kohl van Wijngaarden <ewoud(a)kohlvanwijngaarden.nl>
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com>
Gerrit-Reviewer: Wenyi Gao <wenyi(a)linux.vnet.ibm.com>
8:15 p.m.
Wenyi Gao has posted comments on this change.
Change subject: Add some base functions to vdsm-tool
......................................................................
Patch Set 5: (2 inline comments)
....................................................
File vdsm-tool/base.py
Line 72: def get_config_item(file, section, item, default=None):
Yes, exactly.
Line 76: config.read(file)
I think exceptions here can be catched and handled by the system.
--
To view, visit http://gerrit.ovirt.org/5264
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Id1be737adce28e9ca532aa454e7f0a060947589e
Gerrit-PatchSet: 5
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Wenyi Gao <wenyi(a)linux.vnet.ibm.com>
Gerrit-Reviewer: Adam Litke <agl(a)us.ibm.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Douglas Schilling Landgraf <dougsland(a)redhat.com>
Gerrit-Reviewer: Ewoud Kohl van Wijngaarden <ewoud(a)kohlvanwijngaarden.nl>
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com>
Gerrit-Reviewer: Wenyi Gao <wenyi(a)linux.vnet.ibm.com>
7:26 a.m.
Ewoud Kohl van Wijngaarden has posted comments on this change.
Change subject: Add some base functions to vdsm-tool
......................................................................
Patch Set 5: I would prefer that you didn't submit this
(2 inline comments)
Now I have another question ;)
....................................................
File vdsm-tool/base.py
Line 72: def get_config_item(file, section, item, default=None):
You can call it with a default:
get_config_item('config.ini', 'database', 'host',
'localhost')
If the host item isn't available in the section database you will get
'localhost'. If you didn't give it a default it will return None.
Line 76: config.read(file)
I am wondering if you need to catch any exceptions here though.
--
To view, visit http://gerrit.ovirt.org/5264
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Id1be737adce28e9ca532aa454e7f0a060947589e
Gerrit-PatchSet: 5
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Wenyi Gao <wenyi(a)linux.vnet.ibm.com>
Gerrit-Reviewer: Adam Litke <agl(a)us.ibm.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Douglas Schilling Landgraf <dougsland(a)redhat.com>
Gerrit-Reviewer: Ewoud Kohl van Wijngaarden <ewoud(a)kohlvanwijngaarden.nl>
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com>
Gerrit-Reviewer: Wenyi Gao <wenyi(a)linux.vnet.ibm.com>
7:21 a.m.
Douglas Schilling Landgraf has posted comments on this change.
Change subject: Add some base functions to vdsm-tool
......................................................................
Patch Set 5: I would prefer that you didn't submit this
Adding -1, since I have made a comment/question.
--
To view, visit http://gerrit.ovirt.org/5264
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Id1be737adce28e9ca532aa454e7f0a060947589e
Gerrit-PatchSet: 5
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Wenyi Gao <wenyi(a)linux.vnet.ibm.com>
Gerrit-Reviewer: Adam Litke <agl(a)us.ibm.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Douglas Schilling Landgraf <dougsland(a)redhat.com>
Gerrit-Reviewer: Ewoud Kohl van Wijngaarden <ewoud(a)kohlvanwijngaarden.nl>
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com>
Gerrit-Reviewer: Wenyi Gao <wenyi(a)linux.vnet.ibm.com>
7:20 a.m.
Douglas Schilling Landgraf has posted comments on this change.
Change subject: Add some base functions to vdsm-tool
......................................................................
Patch Set 5: (1 inline comment)
....................................................
File vdsm-tool/base.py
Line 72: def get_config_item(file, section, item, default=None):
What's the purpose of default? Return None only? Sorry, I didn't see it last night
:(
--
To view, visit http://gerrit.ovirt.org/5264
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Id1be737adce28e9ca532aa454e7f0a060947589e
Gerrit-PatchSet: 5
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Wenyi Gao <wenyi(a)linux.vnet.ibm.com>
Gerrit-Reviewer: Adam Litke <agl(a)us.ibm.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Douglas Schilling Landgraf <dougsland(a)redhat.com>
Gerrit-Reviewer: Ewoud Kohl van Wijngaarden <ewoud(a)kohlvanwijngaarden.nl>
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com>
Gerrit-Reviewer: Wenyi Gao <wenyi(a)linux.vnet.ibm.com>
4:04 a.m.
Ewoud Kohl van Wijngaarden has posted comments on this change.
Change subject: Add some base functions to vdsm-tool
......................................................................
Patch Set 5: Looks good to me, but someone else must approve
I think it looks good now.
--
To view, visit http://gerrit.ovirt.org/5264
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Id1be737adce28e9ca532aa454e7f0a060947589e
Gerrit-PatchSet: 5
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Wenyi Gao <wenyi(a)linux.vnet.ibm.com>
Gerrit-Reviewer: Adam Litke <agl(a)us.ibm.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Douglas Schilling Landgraf <dougsland(a)redhat.com>
Gerrit-Reviewer: Ewoud Kohl van Wijngaarden <ewoud(a)kohlvanwijngaarden.nl>
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com>
Gerrit-Reviewer: Wenyi Gao <wenyi(a)linux.vnet.ibm.com>
10:24 p.m.
Douglas Schilling Landgraf has posted comments on this change.
Change subject: Add some base functions to vdsm-tool
......................................................................
Patch Set 3: I would prefer that you didn't submit this
(2 inline comments)
Hi Wenyi, thanks for your patch. I do believe you are going to provide a new version based
on the comments. -1 for while.
Thanks!
....................................................
File vdsm-tool/base.py
Line 43: def call(*args, **kwargs):
+1
Line 64: subprocess.call(['echo', '-n', msg])
+1
--
To view, visit http://gerrit.ovirt.org/5264
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Id1be737adce28e9ca532aa454e7f0a060947589e
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Wenyi Gao <wenyi(a)linux.vnet.ibm.com>
Gerrit-Reviewer: Adam Litke <agl(a)us.ibm.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Douglas Schilling Landgraf <dougsland(a)redhat.com>
Gerrit-Reviewer: Ewoud Kohl van Wijngaarden <ewoud(a)kohlvanwijngaarden.nl>
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com>
Gerrit-Reviewer: Wenyi Gao <wenyi(a)linux.vnet.ibm.com>
8:41 p.m.
Wenyi Gao has posted comments on this change.
Change subject: Add some base functions to vdsm-tool
......................................................................
Patch Set 3: (1 inline comment)
....................................................
File vdsm-tool/base.py
Line 64: subprocess.call(['echo', '-n', msg])
Yes, I ignored the comma. it's ugly, but it works.
Ewoud, you are so professional in python.
--
To view, visit http://gerrit.ovirt.org/5264
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Id1be737adce28e9ca532aa454e7f0a060947589e
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Wenyi Gao <wenyi(a)linux.vnet.ibm.com>
Gerrit-Reviewer: Adam Litke <agl(a)us.ibm.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Douglas Schilling Landgraf <dougsland(a)redhat.com>
Gerrit-Reviewer: Ewoud Kohl van Wijngaarden <ewoud(a)kohlvanwijngaarden.nl>
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com>
Gerrit-Reviewer: Wenyi Gao <wenyi(a)linux.vnet.ibm.com>
9:07 a.m.
Ewoud Kohl van Wijngaarden has posted comments on this change.
Change subject: Add some base functions to vdsm-tool
......................................................................
Patch Set 3: (1 inline comment)
....................................................
File vdsm-tool/base.py
Line 64: subprocess.call(['echo', '-n', msg])
Notice the trailing comma. It removes the trailing '\n'. I will agree it's
ugly.
--
To view, visit http://gerrit.ovirt.org/5264
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Id1be737adce28e9ca532aa454e7f0a060947589e
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Wenyi Gao <wenyi(a)linux.vnet.ibm.com>
Gerrit-Reviewer: Adam Litke <agl(a)us.ibm.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Douglas Schilling Landgraf <dougsland(a)redhat.com>
Gerrit-Reviewer: Ewoud Kohl van Wijngaarden <ewoud(a)kohlvanwijngaarden.nl>
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com>
Gerrit-Reviewer: Wenyi Gao <wenyi(a)linux.vnet.ibm.com>
9 a.m.
Wenyi Gao has posted comments on this change.
Change subject: Add some base functions to vdsm-tool
......................................................................
Patch Set 3: (2 inline comments)
....................................................
File vdsm-tool/base.py
Line 43: def call(*args, **kwargs):
Sounds good.
Line 64: subprocess.call(['echo', '-n', msg])
Actually not. 'echo -n msg' prints msg without '\n', while
'print' prints msg with '\n'
--
To view, visit http://gerrit.ovirt.org/5264
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Id1be737adce28e9ca532aa454e7f0a060947589e
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Wenyi Gao <wenyi(a)linux.vnet.ibm.com>
Gerrit-Reviewer: Adam Litke <agl(a)us.ibm.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Douglas Schilling Landgraf <dougsland(a)redhat.com>
Gerrit-Reviewer: Ewoud Kohl van Wijngaarden <ewoud(a)kohlvanwijngaarden.nl>
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com>
Gerrit-Reviewer: Wenyi Gao <wenyi(a)linux.vnet.ibm.com>
4:30 a.m.
Ewoud Kohl van Wijngaarden has posted comments on this change.
Change subject: Add some base functions to vdsm-tool
......................................................................
Patch Set 3: (4 inline comments)
....................................................
File vdsm-tool/base.py
Line 43: def call(*args, **kwargs):
I think call is a very generic name. Something like call_function is still very generic,
but maybe call_init_function?
Line 49: cmd += ' '.join(['%s=%s' % item for item in
kwargs.iteritems()])
You can remove [ and ] so it becomes a generator:
' '.join('%s=s' % item for item in kwargs.iteritems())
Line 51: cmd += ' ' + arg
You're right, args is a tuple. But you can still use:
cmd += ' ' + ' '.join(args)
Line 64: subprocess.call(['echo', '-n', msg])
I think this is equal to:
print msg,
--
To view, visit http://gerrit.ovirt.org/5264
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Id1be737adce28e9ca532aa454e7f0a060947589e
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Wenyi Gao <wenyi(a)linux.vnet.ibm.com>
Gerrit-Reviewer: Adam Litke <agl(a)us.ibm.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Douglas Schilling Landgraf <dougsland(a)redhat.com>
Gerrit-Reviewer: Ewoud Kohl van Wijngaarden <ewoud(a)kohlvanwijngaarden.nl>
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com>
Gerrit-Reviewer: Wenyi Gao <wenyi(a)linux.vnet.ibm.com>
2:07 a.m.
Wenyi Gao has posted comments on this change.
Change subject: Add some base functions to vdsm-tool
......................................................................
Patch Set 2: (3 inline comments)
....................................................
File vdsm-tool/base.py
Line 40: logging.error(traceback.format_exc())
Good suggestion, thanks.
Line 53: cmd += ' ' + arg
Yes, thanks for your good suggestion, but args is a turpe, not a list.
Line 71: def print_failure_msg(msg):
Yes, good point.
--
To view, visit http://gerrit.ovirt.org/5264
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Id1be737adce28e9ca532aa454e7f0a060947589e
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Wenyi Gao <wenyi(a)linux.vnet.ibm.com>
Gerrit-Reviewer: Adam Litke <agl(a)us.ibm.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Douglas Schilling Landgraf <dougsland(a)redhat.com>
Gerrit-Reviewer: Ewoud Kohl van Wijngaarden <ewoud(a)kohlvanwijngaarden.nl>
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com>
Gerrit-Reviewer: Wenyi Gao <wenyi(a)linux.vnet.ibm.com>
6:12 a.m.
Ewoud Kohl van Wijngaarden has posted comments on this change.
Change subject: Add some base functions to vdsm-tool
......................................................................
Patch Set 2: (3 inline comments)
....................................................
File vdsm-tool/base.py
Line 40: logging.error(traceback.format_exc())
I think we moved away from traceback.format_exc to exc_info=True.
logging.error("Failed to execute '%s', argv, exc_info=True)
Line 53: cmd += ' ' + arg
How about:
cmd = ' '.join([cmd] + ['%s=%s' % i for i in kwargs.iteritems()] + args)
Line 71: def print_failure_msg(msg):
I feel this could be combined with print_success_msg into print_msg. I also think you can
replace the subprocess calls to echo with plain print.
--
To view, visit http://gerrit.ovirt.org/5264
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Id1be737adce28e9ca532aa454e7f0a060947589e
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Wenyi Gao <wenyi(a)linux.vnet.ibm.com>
Gerrit-Reviewer: Adam Litke <agl(a)us.ibm.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Douglas Schilling Landgraf <dougsland(a)redhat.com>
Gerrit-Reviewer: Ewoud Kohl van Wijngaarden <ewoud(a)kohlvanwijngaarden.nl>
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com>
Gerrit-Reviewer: Wenyi Gao <wenyi(a)linux.vnet.ibm.com>
4349
days inactive
4372
days old
vdsm-patches@lists.fedorahosted.org
26 comments
4 participants
participants (4)
-
Dan Kenigsberg -
Douglas Schilling Landgraf -
ewoud＠kohlvanwijngaarden.nl -
wenyi＠linux.vnet.ibm.com