9:10 a.m.
Hello Shmuel Melamud,
I'd like you to do a code review. Please visit
https://gerrit.ovirt.org/42881
to review the following change.
Change subject: virt: Safe device type check for console device
......................................................................
virt: Safe device type check for console device
In Vm._getUnderlyingConsoleDeviceInfo() console device is searched
through the self.conf['devices'] list by checking 'device' field of each
element. Usage of dev['device'] expression for this is unsafe, because
for some devices (RNG device, for example) this key may be absent and
KeyError will be raised. Replaced this by dev.get('device') which is
safe.
Change-Id: I6a0c1adc6c3b2ab8d50b6e3ac712ce2dcf04cc6c
Bug-Url: https://bugzilla.redhat.com/show_bug.cgi?id=1233825
Signed-off-by: Shmuel Melamud <smelamud(a)redhat.com>
---
M vdsm/virt/vm.py
1 file changed, 1 insertion(+), 1 deletion(-)
git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/81/42881/1
diff --git a/vdsm/virt/vm.py b/vdsm/virt/vm.py
index 4977d28..91cb1a0 100644
--- a/vdsm/virt/vm.py
+++ b/vdsm/virt/vm.py
@@ -3908,7 +3908,7 @@
dev.alias = alias
for dev in self.conf['devices']:
- if dev['device'] == hwclass.CONSOLE and \
+ if dev.get('device') == hwclass.CONSOLE and \
not dev.get('alias'):
dev['alias'] = alias
--
To view, visit https://gerrit.ovirt.org/42881
To unsubscribe, visit https://gerrit.ovirt.org/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I6a0c1adc6c3b2ab8d50b6e3ac712ce2dcf04cc6c
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Shmuel Leib Melamud <smelamud(a)redhat.com>
Gerrit-Reviewer: Shmuel Melamud <smelamud(a)redhat.com>
9:10 a.m.
automation(a)ovirt.org has posted comments on this change.
Change subject: virt: Safe device type check for console device
......................................................................
Patch Set 1:
* Update tracker::#1233825::OK
* Check Bug-Url::OK
* Check Public Bug::#1233825::OK, public bug
* Check Product::#1233825::OK, Correct product oVirt
* Check TR::SKIP, not in a monitored branch (ovirt-3.5 ovirt-3.4 ovirt-3.3 ovirt-3.2)
* Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.5',
'ovirt-3.4', 'ovirt-3.3'])
--
To view, visit https://gerrit.ovirt.org/42881
To unsubscribe, visit https://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I6a0c1adc6c3b2ab8d50b6e3ac712ce2dcf04cc6c
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Shmuel Leib Melamud <smelamud(a)redhat.com>
Gerrit-Reviewer: Shmuel Melamud <smelamud(a)redhat.com>
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-HasComments: No
9:14 a.m.
Shmuel Leib Melamud has posted comments on this change.
Change subject: virt: Safe device type check for console device
......................................................................
Patch Set 1: Verified+1
The fix is easy, but the question is how many places exist elsewhere that have the same
problem? Is it desirable to fix them one-by-one, or it's better to do some general
fix? For example, add the 'device' field into the record on VDSM side.
--
To view, visit https://gerrit.ovirt.org/42881
To unsubscribe, visit https://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I6a0c1adc6c3b2ab8d50b6e3ac712ce2dcf04cc6c
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Shmuel Leib Melamud <smelamud(a)redhat.com>
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik <mpolednik(a)redhat.com>
Gerrit-Reviewer: Michal Skrivanek <mskrivan(a)redhat.com>
Gerrit-Reviewer: Shmuel Leib Melamud <smelamud(a)redhat.com>
Gerrit-Reviewer: Shmuel Melamud <smelamud(a)redhat.com>
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-HasComments: No
2:23 a.m.
Francesco Romani has posted comments on this change.
Change subject: virt: Safe device type check for console device
......................................................................
Patch Set 1: Code-Review-1
(1 comment)
-1 for visibility and discussion toward a definitive fix.
https://gerrit.ovirt.org/#/c/42881/1/vdsm/virt/vm.py
File vdsm/virt/vm.py:
Line 3907: if not hasattr(dev, 'alias'):
Line 3908: dev.alias = alias
Line 3909:
Line 3910: for dev in self.conf['devices']:
Line 3911: if dev.get('device') == hwclass.CONSOLE and \
we hit this one time for rng devices, but now I'm starting to see one pattern. The
first question is: shouldn't Engine send 'device' field?
VDSM expects this, as thes BZs demonstrate.
Line 3912: not dev.get('alias'):
Line 3913: dev['alias'] = alias
Line 3914:
Line 3915: def _getUnderlyingSmartcardDeviceInfo(self):
--
To view, visit https://gerrit.ovirt.org/42881
To unsubscribe, visit https://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I6a0c1adc6c3b2ab8d50b6e3ac712ce2dcf04cc6c
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Shmuel Leib Melamud <smelamud(a)redhat.com>
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik <mpolednik(a)redhat.com>
Gerrit-Reviewer: Michal Skrivanek <mskrivan(a)redhat.com>
Gerrit-Reviewer: Shmuel Leib Melamud <smelamud(a)redhat.com>
Gerrit-Reviewer: Shmuel Melamud <smelamud(a)redhat.com>
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-HasComments: Yes
5:19 a.m.
Shmuel Leib Melamud has posted comments on this change.
Change subject: virt: Safe device type check for console device
......................................................................
Patch Set 1:
(1 comment)
https://gerrit.ovirt.org/#/c/42881/1/vdsm/virt/vm.py
File vdsm/virt/vm.py:
Line 3907: if not hasattr(dev, 'alias'):
Line 3908: dev.alias = alias
Line 3909:
Line 3910: for dev in self.conf['devices']:
Line 3911: if dev.get('device') == hwclass.CONSOLE and \
we hit this one time for rng devices, but now I'm starting to see
one patte
From my point of view - yes - the Engine should send both 'device' and
'type' fields for every device.
Line 3912: not
dev.get('alias'):
Line 3913: dev['alias'] = alias
Line 3914:
Line 3915: def _getUnderlyingSmartcardDeviceInfo(self):
--
To view, visit https://gerrit.ovirt.org/42881
To unsubscribe, visit https://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I6a0c1adc6c3b2ab8d50b6e3ac712ce2dcf04cc6c
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Shmuel Leib Melamud <smelamud(a)redhat.com>
Gerrit-Reviewer: Arik Hadas <ahadas(a)redhat.com>
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik <mpolednik(a)redhat.com>
Gerrit-Reviewer: Michal Skrivanek <mskrivan(a)redhat.com>
Gerrit-Reviewer: Omer Frenkel <ofrenkel(a)redhat.com>
Gerrit-Reviewer: Shmuel Leib Melamud <smelamud(a)redhat.com>
Gerrit-Reviewer: Shmuel Melamud <smelamud(a)redhat.com>
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-HasComments: Yes
10:16 a.m.
Martin Polednik has posted comments on this change.
Change subject: virt: Safe device type check for console device
......................................................................
Patch Set 1: Code-Review-1
(1 comment)
https://gerrit.ovirt.org/#/c/42881/1/vdsm/virt/vm.py
File vdsm/virt/vm.py:
Line 3907: if not hasattr(dev, 'alias'):
Line 3908: dev.alias = alias
Line 3909:
Line 3910: for dev in self.conf['devices']:
Line 3911: if dev.get('device') == hwclass.CONSOLE and \
From my point of view - yes - the Engine should send both
'device' and 'typ
Actually it has to be present (it is stated in the
API) and I'm not sure if it's not actually enforced via minimum params validation.
I believe that this fix is not perfectly correct, because it is pretty much swallowing of
an exception that may point out to an error.
Line 3912: not dev.get('alias'):
Line 3913: dev['alias'] = alias
Line 3914:
Line 3915: def _getUnderlyingSmartcardDeviceInfo(self):
--
To view, visit https://gerrit.ovirt.org/42881
To unsubscribe, visit https://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I6a0c1adc6c3b2ab8d50b6e3ac712ce2dcf04cc6c
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Shmuel Leib Melamud <smelamud(a)redhat.com>
Gerrit-Reviewer: Arik Hadas <ahadas(a)redhat.com>
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik <mpolednik(a)redhat.com>
Gerrit-Reviewer: Michal Skrivanek <mskrivan(a)redhat.com>
Gerrit-Reviewer: Omer Frenkel <ofrenkel(a)redhat.com>
Gerrit-Reviewer: Shmuel Leib Melamud <smelamud(a)redhat.com>
Gerrit-Reviewer: Shmuel Melamud <smelamud(a)redhat.com>
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-HasComments: Yes
10:46 a.m.
Shmuel Leib Melamud has posted comments on this change.
Change subject: virt: Safe device type check for console device
......................................................................
Patch Set 1:
(1 comment)
https://gerrit.ovirt.org/#/c/42881/1/vdsm/virt/vm.py
File vdsm/virt/vm.py:
Line 3907: if not hasattr(dev, 'alias'):
Line 3908: dev.alias = alias
Line 3909:
Line 3910: for dev in self.conf['devices']:
Line 3911: if dev.get('device') == hwclass.CONSOLE and \
Actually it has to be present (it is stated in the API) and I'm
not sure if
According to the API spec, 'device' is not present in
VmRngDevice structure and the Engine complies to the spec. But VDSM definitely expects
this parameter, so it should be added.
The second question is do we need to apply this fix to VDSM just for case when it is used
with older Engine.
Line 3912: not dev.get('alias'):
Line 3913: dev['alias'] = alias
Line 3914:
Line 3915: def _getUnderlyingSmartcardDeviceInfo(self):
--
To view, visit https://gerrit.ovirt.org/42881
To unsubscribe, visit https://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I6a0c1adc6c3b2ab8d50b6e3ac712ce2dcf04cc6c
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Shmuel Leib Melamud <smelamud(a)redhat.com>
Gerrit-Reviewer: Arik Hadas <ahadas(a)redhat.com>
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik <mpolednik(a)redhat.com>
Gerrit-Reviewer: Michal Skrivanek <mskrivan(a)redhat.com>
Gerrit-Reviewer: Omer Frenkel <ofrenkel(a)redhat.com>
Gerrit-Reviewer: Shmuel Leib Melamud <smelamud(a)redhat.com>
Gerrit-Reviewer: Shmuel Melamud <smelamud(a)redhat.com>
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-HasComments: Yes
4:11 a.m.
Martin Polednik has posted comments on this change.
Change subject: virt: Safe device type check for console device
......................................................................
Patch Set 1:
(1 comment)
https://gerrit.ovirt.org/#/c/42881/1/vdsm/virt/vm.py
File vdsm/virt/vm.py:
Line 3907: if not hasattr(dev, 'alias'):
Line 3908: dev.alias = alias
Line 3909:
Line 3910: for dev in self.conf['devices']:
Line 3911: if dev.get('device') == hwclass.CONSOLE and \
According to the API spec, 'device' is not present in
VmRngDevice structure
Sorry, my failure - this points to an issue in API itself
rather than the code. If the device is sent from engine correctly at this time, then I
suggest fixing the API to correctly specify the device rather than getting in between
spec.
Line 3912: not dev.get('alias'):
Line 3913: dev['alias'] = alias
Line 3914:
Line 3915: def _getUnderlyingSmartcardDeviceInfo(self):
--
To view, visit https://gerrit.ovirt.org/42881
To unsubscribe, visit https://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I6a0c1adc6c3b2ab8d50b6e3ac712ce2dcf04cc6c
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Shmuel Leib Melamud <smelamud(a)redhat.com>
Gerrit-Reviewer: Arik Hadas <ahadas(a)redhat.com>
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik <mpolednik(a)redhat.com>
Gerrit-Reviewer: Michal Skrivanek <mskrivan(a)redhat.com>
Gerrit-Reviewer: Omer Frenkel <ofrenkel(a)redhat.com>
Gerrit-Reviewer: Shmuel Leib Melamud <smelamud(a)redhat.com>
Gerrit-Reviewer: Shmuel Melamud <smelamud(a)redhat.com>
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-HasComments: Yes
9:26 a.m.
Shmuel Leib Melamud has abandoned this change.
Change subject: virt: Safe device type check for console device
......................................................................
Abandoned
Needs to be fixed in Engine and API spec corrected.
--
To view, visit https://gerrit.ovirt.org/42881
To unsubscribe, visit https://gerrit.ovirt.org/settings
Gerrit-MessageType: abandon
Gerrit-Change-Id: I6a0c1adc6c3b2ab8d50b6e3ac712ce2dcf04cc6c
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Shmuel Leib Melamud <smelamud(a)redhat.com>
Gerrit-Reviewer: Arik Hadas <ahadas(a)redhat.com>
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik <mpolednik(a)redhat.com>
Gerrit-Reviewer: Michal Skrivanek <mskrivan(a)redhat.com>
Gerrit-Reviewer: Omer Frenkel <ofrenkel(a)redhat.com>
Gerrit-Reviewer: Shmuel Leib Melamud <smelamud(a)redhat.com>
Gerrit-Reviewer: Shmuel Melamud <smelamud(a)redhat.com>
Gerrit-Reviewer: automation(a)ovirt.org
9:26 a.m.
automation(a)ovirt.org has posted comments on this change.
Change subject: virt: Safe device type check for console device
......................................................................
Patch Set 1:
* Update tracker::#1233825::OK
--
To view, visit https://gerrit.ovirt.org/42881
To unsubscribe, visit https://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I6a0c1adc6c3b2ab8d50b6e3ac712ce2dcf04cc6c
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Shmuel Leib Melamud <smelamud(a)redhat.com>
Gerrit-Reviewer: Arik Hadas <ahadas(a)redhat.com>
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik <mpolednik(a)redhat.com>
Gerrit-Reviewer: Michal Skrivanek <mskrivan(a)redhat.com>
Gerrit-Reviewer: Omer Frenkel <ofrenkel(a)redhat.com>
Gerrit-Reviewer: Shmuel Leib Melamud <smelamud(a)redhat.com>
Gerrit-Reviewer: Shmuel Melamud <smelamud(a)redhat.com>
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-HasComments: No
3014
days inactive
3021
days old
vdsm-patches@lists.fedorahosted.org
9 comments
4 participants
participants (4)
-
automation＠ovirt.org -
fromani＠redhat.com -
Martin Polednik -
smelamud＠redhat.com