Antoni Segura Puimedon has posted comments on this change.

Change subject: Integrate Smartcard support
......................................................................


Patch Set 15: (4 inline comments)

Just a couple of pep8 things. Now I will review the rest.

....................................................
File tests/libvirtvmTests.py
Line 93: 
Line 94:     def testSmartcardXML(self):
Line 95:         smartcardXML = '<smartcard mode="passthrough" 
type="spicevmc"/>'
Line 96:         dev = {'device': 'smartcard',
Line 97:                 'specParams': {'mode': 'passthrough', 'type': 
'spicevmc'}}
The indentation is wrong. It should be:
dev = {'device': 'smartcard',
       'specParams': {'mode': 'passthrough', 'type': 'spicevmc'}}
Line 98:         smartcard = libvirtvm.SmartCardDevice(self.conf, self.log, 
**dev)
Line 99:         self.assertXML(smartcard.getXML(), smartcardXML)
Line 100: 
Line 101:     def testFeaturesXML(self):


....................................................
File vdsm/libvirtvm.py
Line 2636:         Obtain smartcard device info from libvirt.
Line 2637:         """
Line 2638:         smartcardxml = 
_domParseStr(self._lastXMLDesc).childNodes[0]. \
Line 2639:                                  getElementsByTagName('devices')[0]. 
\
Line 2640:                                  getElementsByTagName('smartcard')
The indentation of the previous two lines is wrong. It should be aligned to the 
's' in self.
Line 2641:         for x in smartcardxml:
Line 2642:             if not x.getElementsByTagName('address'):
Line 2643:                 continue
Line 2644: 


Line 2651:                     dev.alias = alias
Line 2652: 
Line 2653:             for dev in self.conf['devices']:
Line 2654:                 if ((dev['device'] == vm.SMARTCARD_DEVICES) and
Line 2655:                     not dev.get('address')):
This is not pep8 compliant. I recommend putting:
if dev['device'] == vm.SMARCARD_DEVICES and \
    not dev.get('address'):
Line 2656:                     dev['address'] = address
Line 2657:                     dev['alias'] = alias
Line 2658: 
Line 2659:     def _getUnderlyingWatchdogDeviceInfo(self):


....................................................
File vdsm/vm.py
Line 554:         if self.conf.get('smartcard'):
Line 555:             cards.append({'device': SMARTCARD_DEVICES,
Line 556:                 'specParams': {
Line 557:                     'mode': 'passthrough',
Line 558:                     'type': 'spicevmc'}})
lines 555-558 are not pep8 compliant. I recommend:

cards.append({'device': SMARTCARD_DEVICES,
              'specParams': {'mode': 'passthrough',
                             'type': 'spicevmc'}})
Line 559:         return cards
Line 560: 
Line 561:     def getConfSound(self):
Line 562:         """


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I7cdaef420c8381d588f6215e66e6a80dd9d2e44b
Gerrit-PatchSet: 15
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Tomas Jelinek <[email protected]>
Gerrit-Reviewer: Antoni Segura Puimedon <[email protected]>
Gerrit-Reviewer: Barak Azulay <[email protected]>
Gerrit-Reviewer: Dan Kenigsberg <[email protected]>
Gerrit-Reviewer: Gal Hammer <[email protected]>
Gerrit-Reviewer: Mark Wu <[email protected]>
Gerrit-Reviewer: Michal Skrivanek <[email protected]>
Gerrit-Reviewer: Peter V. Saveliev <[email protected]>
Gerrit-Reviewer: Tomas Jelinek <[email protected]>
Gerrit-Reviewer: Vinzenz Feenstra <[email protected]>
Gerrit-Reviewer: oVirt Jenkins CI Server
_______________________________________________
vdsm-patches mailing list
[email protected]
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

Reply via email to