GNOME Bugzilla – Bug 666739
Add devices based on what is supported by the OS
Last modified: 2016-03-31 13:56:25 UTC
Add video, sound and tablet devices to new VM based on what is supported by the OS in question. The information about device support comes from libosinfo. Also note that this requires my unmerged patches to libvirt-glib and libosinfo.
Created attachment 204118 [details] [review] Add devices based on what is supported by the OS Add video, sound and tablet devices to new VM based on what is supported by the OS in question. The information about device support comes from libosinfo. Note that we don't add these devices if OS itself or its support for the device in question is unknown and rely on libvirt to fill in the safe defaults for us.
Created attachment 204140 [details] [review] Add devices based on what is supported by the OS Add video, sound and tablet devices to new VM based on what is supported by the OS in question. The information about device support comes from libosinfo. Note that we don't add these devices if OS itself or its support for the device in question is unknown and rely on libvirt to fill in the safe defaults for us.
Review of attachment 204140 [details] [review]: ::: src/vm-configurator.vala @@ +252,3 @@ + + if (val == null) + parents = parents.new_union (clones); I would rather throw an error here. I don't think it's a good idea to silently fail or error out later. otherwise looks good, can you update the patch so it applies in git?
Review of attachment 204140 [details] [review]: ::: src/vm-configurator.vala @@ +247,3 @@ + } + + var upgrades = os.get_related (ProductRelationship.UPGRADES); this function could actually be moved to util.vala, and renamed, no?
(In reply to comment #3) > Review of attachment 204140 [details] [review]: > > ::: src/vm-configurator.vala > @@ +252,3 @@ > + > + if (val == null) > + parents = parents.new_union (clones); > > I would rather throw an error here. > > I don't think it's a good idea to silently fail or error out later. I think you meant this: + static int get_virt_enum_value (string value_nick, Type enum_type) { + var enum_class = (EnumClass) enum_type.class_ref (); + var val = enum_class.get_value_by_nick (value_nick); + + if (val == null) + return -1; The lines you pasted don't exactly exist in my patch. If so I think this is one place where asserts are good for or return_*if_fail() if you prefer. > otherwise looks good, can you update the patch so it applies in git? Sure but I was hoping to move the code for finding all the devices to osinfo but was awaiting Daniel's input on the list to do that in the correct way.
Created attachment 204975 [details] [review] Add devices based on what is supported by the OS Add video, sound and tablet devices to new VM based on what is supported by the OS in question. The information about device support comes from libosinfo. Note that we don't add these devices if OS itself or its support for the device in question is unknown and rely on libvirt to fill in the safe defaults for us.
This new patch makes use of the API I am adding to libosinfo (pending review). I forgot to address the issues you pointed out so i'll submit another version..
Created attachment 204976 [details] [review] Add devices based on what is supported by the OS Add video, sound and tablet devices to new VM based on what is supported by the OS in question. The information about device support comes from libosinfo. Note that we don't add these devices if OS itself or its support for the device in question is unknown and rely on libvirt to fill in the safe defaults for us.
(In reply to comment #7) > This new patch makes use of the API I am adding to libosinfo (pending review). These patches have been reviewed and pushed to libosinfo.
Created attachment 205317 [details] [review] Add devices based on what is supported by the OS Add video, sound and tablet devices to new VM based on what is supported by the OS in question. The information about device support comes from libosinfo. Note that we don't add these devices if OS itself or its support for the device in question is unknown and rely on libvirt to fill in the safe defaults for us.
Review of attachment 205317 [details] [review]: ::: src/vm-configurator.vala @@ +218,3 @@ + } + + var filter = new Filter (); I would move this function to util.vala
(In reply to comment #11) > Review of attachment 205317 [details] [review]: > > ::: src/vm-configurator.vala > @@ +218,3 @@ > + } > + > + var filter = new Filter (); > > I would move this function to util.vala You mean get_device_by_prop() ? Yeah, I guess we should move get_virt_enum_value() there too (and rename of course).
Created attachment 205360 [details] [review] Add devices based on what is supported by the OS Add video, sound and tablet devices to new VM based on what is supported by the OS in question. The information about device support comes from libosinfo. Note that we don't add these devices if OS itself or its support for the device in question is unknown and rely on libvirt to fill in the safe defaults for us.
Review of attachment 205360 [details] [review]: small comment, ack otherwise ::: src/util.vala @@ +254,3 @@ } + public Osinfo.Device? get_os_device_by_prop (Osinfo.Os? os, string prop_name, string prop_value) { this one is less likely to be reused. Perhaps it should be moved to libosinfo instead?
(In reply to comment #14) > Review of attachment 205360 [details] [review]: > > small comment, ack otherwise > > ::: src/util.vala > @@ +254,3 @@ > } > > + public Osinfo.Device? get_os_device_by_prop (Osinfo.Os? os, string > prop_name, string prop_value) { > > this one is less likely to be reused. Perhaps it should be moved to libosinfo > instead? Yeah, already on my TODO. :)
Attachment 205360 [details] pushed as 9dcd91b - Add devices based on what is supported by the OS