Nir Soffer has posted comments on this change.

Change subject: virt: enable libgfapi with snapshot support
......................................................................


Patch Set 1:

(4 comments)

https://gerrit.ovirt.org/#/c/56906/1/vdsm/virt/vm.py
File vdsm/virt/vm.py:

Line 3197
Line 3198
Line 3199
Line 3200
Line 3201
Needs rebase? This does not exist in master.


Line 3192:             hostAttrs = {}
Line 3193: 
Line 3194:             # TODO: unify these with the Drive.getXML() method
Line 3195:             if oldDrive.networkDev:
Line 3196:                 sourceType = 'network'
We have a constant for this, see virt/vmdevices/storage.py
Line 3197:                 sourceAttrs.update({
Line 3198:                     'name': newDriveDict['volumeInfo']['path'],
Line 3199:                     'protocol': 
newDriveDict['volumeInfo']['protocol']})
Line 3200:                 hostAttrs.update({


Line 3195:             if oldDrive.networkDev:
Line 3196:                 sourceType = 'network'
Line 3197:                 sourceAttrs.update({
Line 3198:                     'name': newDriveDict['volumeInfo']['path'],
Line 3199:                     'protocol': 
newDriveDict['volumeInfo']['protocol']})
These should use newDriveDict, not newDriveDict["volumeInto"].
Line 3200:                 hostAttrs.update({
Line 3201:                     'name': 
newDriveDict['volumeInfo']['volfileServer'],
Line 3202:                     'port': newDriveDict['volumeInfo']['port'],
Line 3203:                     'transport': 
newDriveDict['volumeInfo']['transport']})


Line 3199:                     'protocol': 
newDriveDict['volumeInfo']['protocol']})
Line 3200:                 hostAttrs.update({
Line 3201:                     'name': 
newDriveDict['volumeInfo']['volfileServer'],
Line 3202:                     'port': newDriveDict['volumeInfo']['port'],
Line 3203:                     'transport': 
newDriveDict['volumeInfo']['transport']})
This should use Drive.hosts - probably the first host.

Drive.volumeInfo should not be used, it is a leftover from legacy code.

This code should be in vdsm/virt/vmdevices/storage.py. It mostly duplicate 
Drive.getXML().

It should also have tests - see vmStorageTests.py.
Line 3204:             else:
Line 3205:                 sourceAttrs.update({'file': newDriveDict['path']})
Line 3206: 
Line 3207:             disk = vmxml.Element('disk', name=vmDev, 
snapshot='external',


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ibbdedb8bf012fae2f69f4c8c5e952f5eb5ef9132
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Prasanna Kumar Kalever <prasanna.kale...@redhat.com>
Gerrit-Reviewer: Ala Hino <ah...@redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsof...@redhat.com>
Gerrit-Reviewer: gerrit-hooks <automat...@ovirt.org>
Gerrit-HasComments: Yes
_______________________________________________
vdsm-patches mailing list -- vdsm-patches@lists.fedorahosted.org
To unsubscribe send an email to vdsm-patches-le...@lists.fedorahosted.org

Reply via email to