10:23 a.m.
Federico Simoncelli has uploaded a new change for review.
Change subject: Adding the qemuImg command line
......................................................................
Adding the qemuImg command line
Change-Id: I64310c719371a32b1527d9641077315811da9725
---
A vdsm/qemuImg.py
M vdsm/utils.py
2 files changed, 86 insertions(+), 1 deletion(-)
git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/11/2611/1
--
To view, visit http://gerrit.ovirt.org/2611
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I64310c719371a32b1527d9641077315811da9725
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli <fsimonce(a)redhat.com>
3:29 p.m.
Dan Kenigsberg has posted comments on this change.
Change subject: Adding the qemuImg command line
......................................................................
Patch Set 1: I would prefer that you didn't submit this
(1 inline comment)
....................................................
File vdsm/utils.py
Line 600: def execCmd(*args, **kwargs):
you brutally override this. worth an explanation!
--
To view, visit http://gerrit.ovirt.org/2611
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I64310c719371a32b1527d9641077315811da9725
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli <fsimonce(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com>
8:26 a.m.
Federico Simoncelli has posted comments on this change.
Change subject: Adding the qemuImg command line
......................................................................
Patch Set 1: (2 inline comments)
....................................................
File vdsm/qemuImg.py
Line 33: 'backingfile': re.compile("^backing file: (?P<file>.+)
\(actual path"),
It is ok, I don't know exactly what comes after that "(actual path". I know
it's a path, but I'm not interested in it.
....................................................
File vdsm/utils.py
Line 600: def execCmd(*args, **kwargs):
Ah thanks! Sorry I missed it.
--
To view, visit http://gerrit.ovirt.org/2611
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I64310c719371a32b1527d9641077315811da9725
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli <fsimonce(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com>
Gerrit-Reviewer: Igor Lvovsky <ilvovsky(a)redhat.com>
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com>
5:23 p.m.
Federico Simoncelli has posted comments on this change.
Change subject: Adding the qemuImg command line
......................................................................
Patch Set 2: (1 inline comment)
....................................................
File vdsm/utils.py
Line 819: else:
The ideal flow for this method is:
1. if self._cmd is not set, then evaluate and set
2. return self._cmd
Transforming 1 in a new exit point for the function (set and return) is not an
improvement.
The for-else construct is not awkward it's a great tool to avoid an additional flag
variable. It can be used for both patterns "if all elements" and "if any of
the elements". Amazing :-)
--
To view, visit http://gerrit.ovirt.org/2611
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I64310c719371a32b1527d9641077315811da9725
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli <fsimonce(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com>
Gerrit-Reviewer: Igor Lvovsky <ilvovsky(a)redhat.com>
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com>
1:42 p.m.
Dan Kenigsberg has posted comments on this change.
Change subject: Adding the qemuImg command line
......................................................................
Patch Set 2: Looks good to me, but someone else must approve
(2 inline comments)
....................................................
File vdsm/qemuImg.py
Line 24: _qemuimg = CommandPath("qemu-img",
I hope Saggi is now happy
....................................................
File vdsm/utils.py
Line 819: else:
you start to like this awkward syntax too much! ;-)
(I personally do not know what people have against "return" within a loop...)
--
To view, visit http://gerrit.ovirt.org/2611
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I64310c719371a32b1527d9641077315811da9725
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli <fsimonce(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com>
Gerrit-Reviewer: Igor Lvovsky <ilvovsky(a)redhat.com>
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com>
9:47 a.m.
Igor Lvovsky has submitted this change and it was merged.
Change subject: Adding the qemuImg command line
......................................................................
Adding the qemuImg command line
Signed-off-by: Federico Simoncelli <fsimonce(a)redhat.com>
Change-Id: I64310c719371a32b1527d9641077315811da9725
---
M vdsm.spec.in
M vdsm/Makefile.am
A vdsm/qemuImg.py
M vdsm/utils.py
4 files changed, 124 insertions(+), 1 deletion(-)
Approvals:
Saggi Mizrahi: Looks good to me, but someone else must approve
Igor Lvovsky: Verified; Looks good to me, approved
--
To view, visit http://gerrit.ovirt.org/2611
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: I64310c719371a32b1527d9641077315811da9725
Gerrit-PatchSet: 5
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli <fsimonce(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com>
Gerrit-Reviewer: Igor Lvovsky <ilvovsky(a)redhat.com>
Gerrit-Reviewer: Mark Wu <wudxw(a)linux.vnet.ibm.com>
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com>
9:47 a.m.
Igor Lvovsky has posted comments on this change.
Change subject: Adding the qemuImg command line
......................................................................
Patch Set 5: Verified; Looks good to me, approved
--
To view, visit http://gerrit.ovirt.org/2611
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I64310c719371a32b1527d9641077315811da9725
Gerrit-PatchSet: 5
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli <fsimonce(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com>
Gerrit-Reviewer: Igor Lvovsky <ilvovsky(a)redhat.com>
Gerrit-Reviewer: Mark Wu <wudxw(a)linux.vnet.ibm.com>
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com>
12:55 a.m.
Igor Lvovsky has posted comments on this change.
Change subject: Adding the qemuImg command line
......................................................................
Patch Set 5: Looks good to me, but someone else must approve
--
To view, visit http://gerrit.ovirt.org/2611
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I64310c719371a32b1527d9641077315811da9725
Gerrit-PatchSet: 5
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli <fsimonce(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com>
Gerrit-Reviewer: Igor Lvovsky <ilvovsky(a)redhat.com>
Gerrit-Reviewer: Mark Wu <wudxw(a)linux.vnet.ibm.com>
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com>
2:05 a.m.
Mark Wu has posted comments on this change.
Change subject: Adding the qemuImg command line
......................................................................
Patch Set 4: I would prefer that you didn't submit this
(1 inline comment)
....................................................
File vdsm/qemuImg.py
Line 37: rc, out, err = execCmd([_qemuimg.cmd, "info", image])
Why not just use constants.EXT_QEMUIMG? It is configured according to building
enlivenment, so we needn't care about the path of command qemu-img. Do we try to
avoid using "constants" defined in constants.py? What's the benefit?
Thanks
--
To view, visit http://gerrit.ovirt.org/2611
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I64310c719371a32b1527d9641077315811da9725
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli <fsimonce(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com>
Gerrit-Reviewer: Igor Lvovsky <ilvovsky(a)redhat.com>
Gerrit-Reviewer: Mark Wu <wudxw(a)linux.vnet.ibm.com>
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com>
4:09 p.m.
Saggi Mizrahi has posted comments on this change.
Change subject: Adding the qemuImg command line
......................................................................
Patch Set 5: Looks good to me, but someone else must approve
--
To view, visit http://gerrit.ovirt.org/2611
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I64310c719371a32b1527d9641077315811da9725
Gerrit-PatchSet: 5
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli <fsimonce(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com>
Gerrit-Reviewer: Igor Lvovsky <ilvovsky(a)redhat.com>
Gerrit-Reviewer: Mark Wu <wudxw(a)linux.vnet.ibm.com>
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com>
2:44 a.m.
Igor Lvovsky has posted comments on this change.
Change subject: Adding the qemuImg command line
......................................................................
Patch Set 1: (1 inline comment)
....................................................
File vdsm/qemuImg.py
Line 33: 'backingfile': re.compile("^backing file: (?P<file>.+)
\(actual path"),
you missed ')'
--
To view, visit http://gerrit.ovirt.org/2611
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I64310c719371a32b1527d9641077315811da9725
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli <fsimonce(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Igor Lvovsky <ilvovsky(a)redhat.com>
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com>
4403
days inactive
4446
days old
vdsm-patches@lists.fedorahosted.org
10 comments
5 participants
participants (5)
-
Dan Kenigsberg -
Federico Simoncelli -
Igor Lvovsky -
smizrahi＠redhat.com -
wudxw＠linux.vnet.ibm.com