Change in vdsm[master]: cdrom: API change: require interface & index

nsoffer at redhat.com nsoffer at redhat.com
Wed May 11 21:45:11 UTC 2016


Nir Soffer has posted comments on this change.

Change subject: cdrom: API change: require interface & index
......................................................................


Patch Set 3:

(2 comments)

I like the idea of single extensible parameter like the DriveSpecCdrom, even if it is not consistent with older apis.

What is not clear is why engine must send the index - for other drives vdsm select the first available index, why here we use different way?

Do we have a feature page for this explaining the big picture?

https://gerrit.ovirt.org/#/c/56805/3/lib/api/vdsmapi-schema.json
File lib/api/vdsmapi-schema.json:

Line 6497: ##
Line 6498: {'type': 'DriveSpec',
Line 6499:  'data': {},
Line 6500:  'union': ['DriveSpecVolume', 'DriveSpecGUID', 'DriveSpecPayload',
Line 6501:            'DriveSpecPath', 'DriveSpecCdrom', 'str']}
Can you document here the 'str' type, and warn that it is deprecated?

Or, we can add another type like CdromPath, which will be alias to string. In this type we can explain why it exists and that it should never be used.
Line 6502: 
Line 6503: ##
Line 6504: # @VM.changeCD:
Line 6505: #


https://gerrit.ovirt.org/#/c/56805/3/tests/vmTests.py
File tests/vmTests.py:

Line 1598:                 # no specific meaning, actually any error != None is good
Line 1599:                 fakevm._dom = fake.Domain(
Line 1600:                     virtError=libvirt.VIR_ERR_GET_FAILED)
Line 1601: 
Line 1602:                 res = fakevm.changeCD('')
This is confusing, if you are trying to test a libvirt failume, use here a stirng that looks like real cdrom path. This code confuse the read that you test the case when the cdromspec is empty.
Line 1603: 
Line 1604:                 expected_status = define.errCode['changeDisk']['status']
Line 1605:                 self.assertEqual(res['status'], expected_status)
Line 1606: 


-- 
To view, visit https://gerrit.ovirt.org/56805
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I178c1a02bbad962f9dc9b67bed7691cf170ee896
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Martin Polednik <mpolednik at redhat.com>
Gerrit-Reviewer: Francesco Romani <fromani at redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik <mpolednik at redhat.com>
Gerrit-Reviewer: Nir Soffer <nsoffer at redhat.com>
Gerrit-Reviewer: Piotr Kliczewski <piotr.kliczewski at gmail.com>
Gerrit-Reviewer: gerrit-hooks <automation at ovirt.org>
Gerrit-HasComments: Yes


More information about the vdsm-patches mailing list