Change in vdsm[master]: virt: Safe device type check for console device

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 
---
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

[e-mail] 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'])

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.

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):

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):

[Martin Polednik]

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):

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):

[Martin Polednik]

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):

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.

[e-mail] has posted comments on this change.

Change subject: virt: Safe device type check for console device
......................................................................

Patch Set 1:

* Update tracker::#1233825::OK