[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [virt-tools-list] [RFC PATCH 3/3] virtManager: add GUI elements for adding a RNG device



On 09/18/2013 09:29 AM, Giuseppe Scrivano wrote:
> Closes: https://bugzilla.redhat.com/show_bug.cgi?id=1001773
> 
> Signed-off-by: Giuseppe Scrivano <gscrivan redhat com>
> ---
>  ui/vmm-add-hardware.ui     | 235 +++++++++++++++++++++++++++++++++++++++
>  ui/vmm-details.ui          | 271 +++++++++++++++++++++++++++++++++++++++++++++
>  virtManager/addhardware.py | 107 ++++++++++++++++++
>  virtManager/details.py     |  41 ++++++-
>  virtManager/domain.py      |   3 +
>  virtManager/uihelpers.py   |  58 ++++++++++
>  6 files changed, 713 insertions(+), 2 deletions(-)
> 

Please split this patch into two parts: one patch for the details page
handling, and one patch for the addhardware handling.

A few UI comments here:

- On the details page, use a frame like the other UI pages with a bold title.
It's kind of pointless but lets not deviate unnecessarily.

- Switch the details page 'table' to a 'grid'. The only way I found to do this
was to edit the ui file and change the GtkTable to GtkGrid, and then adjust
the spacing in glade (f19 glade doesn't seem to have a grid option). No other
changes should be needed for it to just work.

- Hide the RNG rows in the details page that aren't valid for the rng type.
Once you've converted to grid, you can use the following helper like

uihelpers.set_grid_row_visible(self.widget("rng-device"), not is_egd)

That will hide every element that's in the same row as rng-device, and get the
spacing correct.

- In the details page, drop the 'ypad' for each label, the only row spacing
should be set at the table/grid level and it should be '4' (which it currently
is). I realize this was just following the TPM example as well, but it needs
to be fixed as described above.

- In addhardware, you use a notebook to separate the different UI for
type=device vs type=egd. I'd recommend just using a single grid to hold all
the elements, and doing the set_grid_row_visible trick. We use this on other
pages, and it has the advantage that you don't need to duplicate UI fields
across the notebook pages if there are shared properties (doesn't apply for
rng but still). It also gives proper alignment for the Type: field WRT the
rest of the UI elements.

Some code comments below:

> diff --git a/virtManager/addhardware.py b/virtManager/addhardware.py
> index e13482c..0fe6d06 100644
> --- a/virtManager/addhardware.py
> +++ b/virtManager/addhardware.py
> @@ -54,6 +54,7 @@ PAGE_FILESYSTEM = 10
>  PAGE_SMARTCARD = 11
>  PAGE_USBREDIR = 12
>  PAGE_TPM = 13
> +PAGE_RNG = 14
>  
>  char_widget_mappings = {
>      "source_path" : "char-path",
> @@ -105,6 +106,8 @@ class vmmAddHardware(vmmGObjectUI):
>              "on_fs_source_browse_clicked": self.browse_fs_source,
>  
>              "on_usbredir_type_changed": self.change_usbredir_type,
> +
> +            "on_rng_type_changed": self.change_rng_type,
>          })
>          self.bind_escape_key_close()
>  
> @@ -310,6 +313,14 @@ class vmmAddHardware(vmmGObjectUI):
>          combo = self.widget("tpm-type")
>          uihelpers.build_tpm_type_combo(self.vm, combo)
>  
> +        # RNG widgets
> +        combo = self.widget("rng-type")
> +        uihelpers.build_rng_type_combo(self.vm, combo)
> +        combo = self.widget("rng-backend-type")
> +        uihelpers.build_rng_backend_type_combo(self.vm, combo)
> +        combo = self.widget("rng-backend-mode")
> +        uihelpers.build_rng_backend_mode_combo(self.vm, combo)
> +

I usually only stick these bits in uihelpers if they are actually used in two
places (usually addhw and details). I don't think there's much use in allowing
to editing an RNG device in details, so this stuff likely never needs to be
shared. Just track it in addhardware.py

>          # Available HW options
>          is_local = not self.conn.is_remote()
>          is_storage_capable = self.conn.is_storage_capable()
> @@ -376,6 +387,7 @@ class vmmAddHardware(vmmGObjectUI):
>                        True, None)
>          add_hw_option("TPM", "device_cpu", PAGE_TPM,
>                        True, None)
> +        add_hw_option("RNG", "system-run", PAGE_RNG, True, None)
>  
>      def reset_state(self):
>          # Storage init
> @@ -474,6 +486,11 @@ class vmmAddHardware(vmmGObjectUI):
>              widget = notebook.get_nth_page(page)
>              widget.hide()
>  
> +        # RNG params
> +        self.widget("rng-device").set_text("/dev/random")
> +        self.widget("rng-host").set_text("localhost")
> +        self.widget("rng-service").set_text("708")
> +
>          self.set_hw_selection(0)
>  
>      #########################
> @@ -771,6 +788,44 @@ class vmmAddHardware(vmmGObjectUI):
>          typestr = typ.get_model().get_value(typ.get_active_iter(), 0)
>          return typestr
>  
> +    # RNG getters
> +    def get_config_rng_type(self):
> +        src = self.widget("rng-type")
> +        idx = src.get_active()
> +        if idx < 0:
> +            return None
> +
> +        selected_type = src.get_model()[idx][0]
> +        return selected_type
> +
> +    def get_config_rng_device(self):
> +        if self.get_config_rng_type() == virtinst.VirtualRNGDevice.TYPE_RANDOM:
> +            return self.widget("rng-device").get_text()
> +
> +        return None
> +
> +    def get_config_rng_host(self):
> +        if self.get_config_rng_type() == virtinst.VirtualRNGDevice.TYPE_EGD:
> +            return self.widget("rng-host").get_text()
> +
> +        return None
> +
> +    def get_config_rng_service(self):
> +        if self.get_config_rng_type() == virtinst.VirtualRNGDevice.TYPE_EGD:
> +            return self.widget("rng-service").get_text()
> +
> +        return None
> +
> +    def get_config_rng_backend_type(self):
> +        active = self.widget("rng-backend-type").get_active()
> +        model = self.widget("rng-backend-type").get_model()
> +        return model[active][0]
> +
> +    def get_config_rng_backend_mode(self):
> +        active = self.widget("rng-backend-mode").get_active()
> +        model = self.widget("rng-backend-mode").get_model()
> +        return model[active][0]
> +
>      ################
>      # UI listeners #
>      ################
> @@ -929,6 +984,8 @@ class vmmAddHardware(vmmGObjectUI):
>              return _("USB Redirection")
>          if page == PAGE_TPM:
>              return _("TPM")
> +        if page == PAGE_RNG:
> +            return _("Random Number Generator")
>  
>          if page == PAGE_CHAR:
>              char_class = self.get_char_type()
> @@ -990,6 +1047,14 @@ class vmmAddHardware(vmmGObjectUI):
>          uihelpers.set_grid_row_visible(self.widget("usbredir-host-box"),
>                                         showhost)
>  
> +    def change_rng_type(self, ignore1):
> +        model = self.get_config_rng_type()
> +        if model is None:
> +            return
> +
> +        page_idx = virtinst.VirtualRNGDevice.TYPES.index(model)
> +        self.widget("rng-host-notebook").set_current_page(page_idx)
> +
>      # FS listeners
>      def browse_fs_source(self, ignore1):
>          self._browse_file(self.widget("fs-source"), isdir=True)
> @@ -1166,6 +1231,8 @@ class vmmAddHardware(vmmGObjectUI):
>              return self.validate_page_usbredir()
>          elif page_num == PAGE_TPM:
>              return self.validate_page_tpm()
> +        elif page_num == PAGE_RNG:
> +            return self.validate_page_rng()
>  
>      def validate_page_storage(self):
>          bus, device = self.get_config_disk_target()
> @@ -1539,6 +1606,46 @@ class vmmAddHardware(vmmGObjectUI):
>          except Exception, e:
>              return self.err.val_err(_("TPM device parameter error"), e)
>  
> +    def validate_page_rng(self):
> +        conn = self.conn.get_backend()
> +        model = self.get_config_rng_type()
> +
> +        if model == virtinst.VirtualRNGDevice.TYPE_RANDOM:
> +            if not self.get_config_rng_device():
> +                return self.err.val_err(_("RNG selection error."),
> +                                    _("A device must be specified."))
> +        elif model == virtinst.VirtualRNGDevice.TYPE_EGD:
> +            if not self.get_config_rng_host():
> +                return self.err.val_err(_("RNG selection error."),
> +                                    _("The EGD host must be specified."))
> +
> +            if not self.get_config_rng_service():
> +                return self.err.val_err(_("RNG selection error."),
> +                                    _("The EGD service must be specified."))
> +        else:
> +            return self.err.val_err(_("RNG selection error."),
> +                                    _("Invalid RNG type."))
> +
> +        value_mappings = {
> +            "backend_mode" : "connect",
> +            "backend_type" : self.get_config_rng_backend_type(),
> +            "backend_source_mode" : self.get_config_rng_backend_mode(),
> +            "backend_source_host" : self.get_config_rng_host(),
> +            "backend_source_service" : self.get_config_rng_service(),
> +            "device" : self.get_config_rng_device(),
> +            "model" : "virtio"
> +        }
> +

I'd drop the explicit model=virtio and just let the HV fill it in. Hardcoding
things like sometimes comes back to haunt us.

> +        try:
> +            self._dev = virtinst.VirtualRNGDevice(conn)
> +            self._dev.type = self.get_config_rng_type()
> +            for param_name, val in value_mappings.items():
> +                if self._dev.supports_property(param_name):
> +                    setattr(self._dev, param_name, val)
> +        except Exception, e:
> +            return self.err.val_err(_("TPM device parameter error"), e)
> +
> +
>      ####################
>      # Unsorted helpers #
>      ####################
> diff --git a/virtManager/details.py b/virtManager/details.py
> index a02778c..d08ef71 100644
> --- a/virtManager/details.py
> +++ b/virtManager/details.py
> @@ -122,14 +122,16 @@ EDIT_TPM_TYPE,
>   HW_LIST_TYPE_FILESYSTEM,
>   HW_LIST_TYPE_SMARTCARD,
>   HW_LIST_TYPE_REDIRDEV,
> - HW_LIST_TYPE_TPM) = range(19)
> + HW_LIST_TYPE_TPM,
> + HW_LIST_TYPE_RNG) = range(20)
>  
>  remove_pages = [HW_LIST_TYPE_NIC, HW_LIST_TYPE_INPUT,
>                  HW_LIST_TYPE_GRAPHICS, HW_LIST_TYPE_SOUND, HW_LIST_TYPE_CHAR,
>                  HW_LIST_TYPE_HOSTDEV, HW_LIST_TYPE_DISK, HW_LIST_TYPE_VIDEO,
>                  HW_LIST_TYPE_WATCHDOG, HW_LIST_TYPE_CONTROLLER,
>                  HW_LIST_TYPE_FILESYSTEM, HW_LIST_TYPE_SMARTCARD,
> -                HW_LIST_TYPE_REDIRDEV, HW_LIST_TYPE_TPM]
> +                HW_LIST_TYPE_REDIRDEV, HW_LIST_TYPE_TPM,
> +                HW_LIST_TYPE_RNG]
>  
>  # Boot device columns
>  (BOOT_DEV_TYPE,
> @@ -1256,6 +1258,8 @@ class vmmDetails(vmmGObjectUI):
>                  self.refresh_redir_page()
>              elif pagetype == HW_LIST_TYPE_TPM:
>                  self.refresh_tpm_page()
> +            elif pagetype == HW_LIST_TYPE_RNG:
> +                self.refresh_rng_page()
>              else:
>                  pagetype = -1
>          except Exception, e:
> @@ -3143,6 +3147,34 @@ class vmmDetails(vmmGObjectUI):
>          # Device type specific properties, only show if apply to the cur dev
>          show_ui("device_path")
>  
> +    def refresh_rng_page(self):
> +        dev = self.get_hw_selection(HW_LIST_COL_DEVICE)
> +        values = {"rng-type" : "type",
> +                  "rng-device" : "device",
> +                  "rng-host" : "backend_source_host",
> +                  "rng-service" : "backend_source_service",
> +                  "rng-mode" : "backend_source_mode",
> +                  "rng-backend-type" : "backend_type",
> +                  "rng-rate-bytes" : "rate_bytes",
> +                  "rng-rate-period" : "rate_period"}
> +        rewriter = {"rng-type" : lambda x:
> +                    virtinst.VirtualRNGDevice.get_pretty_type(x),
> +                  "rng-backend-type" : lambda x:
> +                    virtinst.VirtualRNGDevice.get_pretty_backend_type(x),
> +                  "rng-mode" : lambda x:
> +                    virtinst.VirtualRNGDevice.get_pretty_mode(x)
> +        }

I prefer spacing like

rewriter = {
    foo: bar,
}

though you'll probably find examples in the code of your format :)

> +
> +        for k in values:

You can do 'for k, prop in values.items()'

Thanks,
Cole


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]