[PATCH virt-manager v3] virt-manager: prevent attach of inactive nodedevs

[Cole Robinson]

On 2/1/22 8:43 AM, Shalini Chellathurai Saroja wrote:
> With virt-manager application, it is possible to add inactive node
> devices(eg: mediated device) in host system to guest system. But it is
> impossible to start a guest system with inactive node devices.  Also,
> it is not yet possible to start a node device with virt-manager
> application. So, the user cannot use the inactive node devices.
> 
> This patch disables the "finish" button and provides a tip, when
> inactive node devices are selected. So, it is not possible to add
> inactive node devices to the guest system.
> 
> Signed-off-by: Shalini Chellathurai Saroja <shalini at linux.ibm.com>
> Reviewed-by: Boris Fiuczynski <fiuczy at linux.ibm.com>
> ---
>  virtManager/addhardware.py | 42 +++++++++++++++++++++++++++++++++++---
>  1 file changed, 39 insertions(+), 3 deletions(-)
> 
> diff --git a/virtManager/addhardware.py b/virtManager/addhardware.py
> index 132ba4e0..60b58741 100644
> --- a/virtManager/addhardware.py
> +++ b/virtManager/addhardware.py
> @@ -754,7 +754,7 @@ class vmmAddHardware(vmmGObjectUI):
>      def _build_hostdev_treeview(self):
>          host_dev = self.widget("host-device")
>          # [ xmlobj, label]
> -        host_dev_model = Gtk.ListStore(object, str)
> +        host_dev_model = Gtk.ListStore(object, str, bool)
>          host_dev.set_model(host_dev_model)
>          host_col = Gtk.TreeViewColumn()
>          text = Gtk.CellRendererText()

Add this here to make the text visibly insensitive:

diff --git a/virtManager/addhardware.py b/virtManager/addhardware.py
index 60b58741..5e52c1ea 100644
--- a/virtManager/addhardware.py
+++ b/virtManager/addhardware.py
@@ -760,6 +760,7 @@ class vmmAddHardware(vmmGObjectUI):
         text = Gtk.CellRendererText()
         host_col.pack_start(text, True)
         host_col.add_attribute(text, 'text', 1)
+        host_col.add_attribute(text, 'sensitive', 2)
         host_dev_model.set_sort_column_id(1, Gtk.SortType.ASCENDING)
         host_dev.append_column(host_col)


> @@ -763,6 +763,28 @@ class vmmAddHardware(vmmGObjectUI):
>          host_dev_model.set_sort_column_id(1, Gtk.SortType.ASCENDING)
>          host_dev.append_column(host_col)
>  
> +
> +    def disable_finish_if_inactive(self, selection):

Please name this _hostdev_row_selected_cb or something describing the
context it is called from.

> +        model, row = selection.get_selected()
> +

the 'row' here is a 'treeiter'

> +        if row is None:
> +            return
> +
> +        hostdev = model[row]

calling this variable 'hostdev' is confusing. This should be called
'row' IMO


> +        if hostdev[1] is None:
> +            self.widget("create-finish").set_sensitive(False)
> +            self.widget("create-finish").set_tooltip_text()
> +        elif hostdev[2]:
> +            self.widget("create-finish").set_sensitive(True)
> +            self.widget("create-finish").set_tooltip_text()
> +        else:
> +            tooltip = (_("%s is not active in the host system.\n"
> +            "Please start the mdev in the host system before adding it to the guest.")
> +            % hostdev[1])
> +            self.widget("create-finish").set_sensitive(False)
> +            self.widget("create-finish").set_tooltip_text(tooltip)
> +

I prefer if there's only a single set_sensitive and set_tooltip_text
call. The state is basically

tooltip = None
sensitive = row[2]
if row[1] and sensitive is False:
    tooltip = ...
set_tooltip_text(tooltip)
set_sensitive(sensitive)

> +
>      def _populate_hostdev_model(self, devtype):
>          devlist = self.widget("host-device")
>          model = devlist.get_model()
> @@ -790,12 +812,26 @@ class vmmAddHardware(vmmGObjectUI):
>                          prettyname = "%s %s" % (
>                                  parentdev.pretty_name(), prettyname)
>  
> -            model.append([dev.xmlobj, prettyname])
> +            model.append([dev.xmlobj, prettyname, dev.is_active()])
>  
>          if len(model) == 0:
> -            model.append([None, _("No Devices Available")])
> +            model.append([None, _("No Devices Available"), False])
> +
>          uiutil.set_list_selection_by_number(devlist, 0)
>  
> +        # enable/disable finish button for default selection
> +        if model[0][2]:
> +            self.widget("create-finish").set_sensitive(True)
> +            self.widget("create-finish").set_tooltip_text()
> +        else:
> +            tooltip = (_("%s is not active in the host system.\n"
> +            "Please start the mdev in the host system before adding it to the guest.")
> +            % model[0][1])
> +            self.widget("create-finish").set_sensitive(False)
> +            self.widget("create-finish").set_tooltip_text(tooltip)
> +
> +        devlist.get_selection().connect("changed", self.disable_finish_if_inactive)
> +

Please break this long line here.

Duplication like that is a sign that something is wrong :)
You can trigger the callback by doing:

devlist.get_selection().emit("changed")

Thanks,
Cole