Hans De Goede has posted comments on this change.
Change subject: Add support for redir devices ......................................................................
Patch Set 2:
Hi again,
I'm sorry my review comment from yesterday may have been a bit harsh. It is just that I'm a bit surprised that after working fulltime on this for a week, and then discussing it for a further week, so after 2 weeks something as fundamental as the API comes up for discussion. That is something to discuss on day 1, not after 2 weeks.
The (xmlrpc) API is the boundary and interface between ovirt-engine and vdsm, and it is important that such interfaces are well designed and simple to understand. And it seems clear to me that e.g.: devices='{type:redir,device:spicevmc,bus:usb,address:{type:usb,bus:0,port:2}}' Is simply just clearer and simpler then: devices='{type:redir,device:spicevmc,specParams{bus:usb},address:{type:usb,bus:0,port:2}}' And also maps 1 on 1 to libvirt's xml, making it easy for users when troubleshooting to see the relation between the xmlrpc call contents and the generated libvirt xml.
I trust that you've good reasons to store certain values inside ovirt-engine inside a specParams dict, but there is no reason that those internals should leak into an boundary API / interface. On the contrary such API's should isolate both sides from the other sides internals!
Luckily there is an easy solution, ovirt can simply keep using the specParams internally, and when it does a create call to vdsm, it can make a copy of the devices array and then for each device in the copy take the contents of the specParams dict of that device, add those to the device's own dict and remove the specParams. And then use the modified devices copy to pass to the create call. This way we can stop the ovirt-engine specParams internal from leaking into the API between vdsm and ovirt-engine and thus keep the API sane.
I know that specParams currently is already used for the vram of video devices, and this currently requires an ugly hack inside vdsm, prooving that having specParams in the API is a BAD idea. It will be quite easy to add support for having a vram value directly inside the device dict, and vdsm can keep support for the specParams for video devices for a while as a transition phase, and then later on delete the ugly hack :)
I would be more then happy to write a vdsm patch to add support for having vram as a value on the device dict level.
Regards,
Hans
-- To view, visit http://gerrit.ovirt.org/4133 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: Ia91922a0f34cb4b32efa2c6397d13fe59aec04e7 Gerrit-PatchSet: 2 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Hans De Goede hdegoede@redhat.com Gerrit-Reviewer: Eli Mesika emesika@redhat.com Gerrit-Reviewer: Gal Hammer ghammer@redhat.com Gerrit-Reviewer: Hans De Goede hdegoede@redhat.com Gerrit-Reviewer: Igor Lvovsky ilvovsky@redhat.com Gerrit-Reviewer: Oved Ourfali oourfali@redhat.com