Dan Kenigsberg has posted comments on this change.

Change subject: vdsm_hooks/hostusb: Allow multiple usb devices with same vendor 
and product id
......................................................................


Patch Set 11: Code-Review-1

(1 comment)

http://gerrit.ovirt.org/#/c/14237/11/vdsm_hooks/hostusb/before_vm_start.py
File vdsm_hooks/hostusb/before_vm_start.py:

Line 133:                             sys.stderr.write('hostusb: bad input, 
expected '
Line 134:                                              '000 format for bus and 
device '
Line 135:                                              'id, input: %s:%s\n' %
Line 136:                                              (fbusid, fphydevid))
Line 137:                             sys.exit(2)
the orginal code already had it, but this patch makes it worse by adding more 
exit(2). The code should raise an exception on error, that should be handled 
once  in the except block.

I would be grateful if the code is transformed to current "standard" of hook 
writing, with a main() function and a test() function. See recently accepted 
http://gerrit.ovirt.org/#/c/22178/11/vdsm_hooks/spiceoptions/before_vm_start.py
Line 138: 
Line 139:             elif not usefilter:
Line 140:                 vendorid, productid = usb.split(':')
Line 141: 


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I831d047e5a2284dcc2e4db7608a9831e64f9c8dc
Gerrit-PatchSet: 11
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: David Guglielmi <david.guglie...@gmail.com>
Gerrit-Reviewer: Antoni Segura Puimedon <asegu...@redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <dan...@redhat.com>
Gerrit-Reviewer: David Guglielmi <david.guglie...@gmail.com>
Gerrit-Reviewer: Francesco Romani <from...@redhat.com>
Gerrit-Reviewer: Itamar Heim <ih...@redhat.com>
Gerrit-Reviewer: Martin Polednik <mpole...@redhat.com>
Gerrit-Reviewer: Vinzenz Feenstra <vfeen...@redhat.com>
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes
_______________________________________________
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

Reply via email to