[PATCH rhel7-branch 1/4] network: GUI, handle virtual devices (bond, vlan, team) properly (#1036047)

Radek Vykydal rvykydal at redhat.com
Wed Dec 4 10:00:15 UTC 2013


On 12/02/2013 01:37 PM, Vratislav Podzimek wrote:
> On Fri, 2013-11-29 at 13:17 +0100, Radek Vykydal wrote:
>> @@ -352,30 +434,28 @@ class NetworkControlBox(object):
>>               selection.select_iter(itr)
>>   
>>       def refresh(self):
>> -        device = self.selected_device()
>> -        self.refresh_ui(device)
>> +        self.refresh_ui()
>>   
>>       # Signal handlers.
>> -    def on_device_selection_changed(self, *args):
>> -        device = self.selected_device()
>> -        if not device:
>> -            return
>> +    def on_nm_state_changed(self, *args):
>> +        self.emit("nm-state-changed")
>>   
>> -        log.debug("network: selected device %s" % device.get_iface())
>> -        self.refresh_ui(device)
>> +    def on_device_selection_changed(self, *args):
>> +        self.refresh_ui()
>>   
>> -    def on_device_state_changed(self, *args):
>> -        device = args[0]
>> -        new_state = args[1]
>> +    def on_device_state_changed(self, device, new_state, *args):
>> +        self.emit("device-state-changed", device.get_iface(), new_state, *args)
>>           if new_state == NetworkManager.DeviceState.SECONDARIES:
>>               return
>>           self._refresh_carrier_info()
>> -        if device == self.selected_device():
>> -            self.refresh_ui(device, new_state)
>> +        dev_cfg = self.selected_dev_cfg()
>> +        if dev_cfg and dev_cfg.device == device:
>> +            self.refresh_ui(state=new_state)
>>   
>>       def on_device_config_changed(self, device, *args):
>> -        if device == self.selected_device():
>> -            self.refresh_ui(device)
>> +        dev_cfg = self.selected_dev_cfg()
>> +        if dev_cfg and dev_cfg.device == device:
>> +            self.refresh_ui()
>>   
>>       def on_wireless_ap_changed_cb(self, combobox, *args):
>>           if self._updating_device:
>> @@ -384,13 +464,13 @@ class NetworkControlBox(object):
>>           if not iter:
>>               return
>>   
>> -        device = self.selected_device()
>> +        device = self.selected_dev_cfg().device
> The value returned from self.selected_dev_cfg() should be checked for
> not being None first. At least that's what is done in all the other
> places.
>

Unlike in other places the race condition the check is for shouldn't
occur here, but adding the check to be on safe side won't hurt.

>> @@ -483,28 +555,29 @@ class NetworkControlBox(object):
>>               return
>>   
>>           active = switch.get_active()
>> -        device = self.selected_device()
>> +        dev_cfg = self.selected_dev_cfg()
> Same check here? Or is it for sure some device will be selected here?
> `

I'd think so, but I'll add the check.

>>   
>>           log.info("network: device %s switched %s" %
>> -                  (device.get_iface(), "on" if active else "off"))
>> +                   (dev_cfg.get_iface(), "on" if active else "off"))
>>   
>> -        dev_type = device.get_device_type()
>> -        if dev_type in self.wired_ui_device_types:
>> +        if dev_cfg.device_type == NetworkManager.DeviceType.WIFI:
>> +            self.client.wireless_set_enabled(active)
>> +        else:
>>               if active:
>> -                uuid = nm.nm_device_setting_value(device.get_iface(), "connection", "uuid")
>> -                if uuid:
>> -                    try:
>> -                        nm.nm_activate_device_connection(device.get_iface(), uuid)
>> -                    except nm.UnmanagedDeviceError as e:
>> -                        log.debug("network: on_device_off_toggled: %s" % e)
>> -                else:
>> -                    self.client.add_and_activate_connection(None, device, None,
>> -                                                            None, None)
>> +                dev_name = dev_cfg.device and dev_cfg.device.get_iface()
>> +                if not dev_cfg.con_uuid:
>> +                    log.debug("network: on_device_off_toggled: no connection for %s"
>> +                               % dev_name)
>> +                    return
>> +                try:
>> +                    nm.nm_activate_device_connection(dev_name, dev_cfg.con_uuid)
>> +                except (nm.UnmanagedDeviceError, nm.UnknownDeviceError) as e:
>> +                    log.debug("network: on_device_off_toggled: %s" % e)
>>               else:
>> -                device.disconnect(None, None)
>> -        elif dev_type == NetworkManager.DeviceType.WIFI:
>> -            self.client.wireless_set_enabled(active)
>> -
>> +                try:
>> +                    nm.nm_disconnect_device(dev_cfg.get_iface())
>> +                except (nm.UnmanagedDeviceError, nm.DeviceNotActiveError) as e:
>> +                    log.debug("network: on_device_off_toggled: %s" % e)
>>   
>>       def on_add_device_clicked(self, *args):
>>           dialog = self.builder.get_object("add_device_dialog")
>> @@ -532,130 +605,129 @@ class NetworkControlBox(object):
>>                   self._running_nmce = None
>>               network.logIfcfgFiles("nm-c-e run")
>>   
>> -    def selected_device(self):
>> +    def selected_dev_cfg(self):
>>           selection = self.builder.get_object("treeview_devices").get_selection()
>> -        (model, iter) = selection.get_selected()
>> +        model, iter = selection.get_selected()
>>           if not iter:
>>               return None
>> -        return model.get(iter, DEVICES_COLUMN_OBJECT)[0]
>> +        return model[iter][DEVICES_COLUMN_OBJECT]
>>   
>> -    def _device_is_stored(self, nm_device):
>> -        """Check that device with Udi of nm_device is already in liststore"""
>> -        udi = nm_device.get_udi()
>> -        model = self.builder.get_object("liststore_devices")
>> -        for row in model:
>> -            if udi == row[DEVICES_COLUMN_OBJECT].get_udi():
>> -                return True
>> -        return False
>> +    def add_dev_cfg(self, dev_cfg):
>> +        log.debug ("network: GUI, device configuration added: connection %s device %s"
>> +                     % (dev_cfg.con_uuid, dev_cfg.get_iface()))
>> +        self.builder.get_object("liststore_devices").append([
>> +            self._dev_icon_name(dev_cfg),
>> +            self.device_type_sort_value.get(dev_cfg.device_type, "100"),
>> +            self._dev_title(dev_cfg),
>> +            dev_cfg
>> +        ])
> The "liststore_devices" store should be pulled only once from the
> builder, not with every single item added.

Okay, adding the item happens only a couple of times,
but the dev_cfg below is more frequent, and other places
are calling for it too, I'll add it.

> +        if dev_cfg.device:
> +            if (dev_cfg.device.get_state() == NetworkManager.DeviceState.UNAVAILABLE
> +                and dev_cfg.device.get_device_type() == NetworkManager.DeviceType.ETHERNET
> +                and not dev_cfg.device.get_carrier()):
> +                # Translators: ethernet cable is unplugged
> We use TRANSLATORS in other places.

I'll make it consistent.

> -            title = ""
> -        return title
> +    def dev_cfg(self, uuid=None, device=None):
> +        model = self.builder.get_object("liststore_devices")
> Should be self._liststore_devices pulled only once.

Yep.

>           # TODO: is this necessary? Isn't it static from glade?
>           self.builder.get_object("label_%s_device" % dev_type_str).set_label(
> -            "%s (%s)" % (self._dev_type_str(device), device.get_iface()))
> +            "%s (%s)" % (self.device_type_name.get(dev_cfg.device_type, ""), dev_cfg.get_iface()))
> I think a temporary variable for the label would clarify this piece of
> code a bit.

I agree.

>   
>   if __name__ == "__main__":
> -
>       win = Gtk.Window()
>       win.connect("delete-event", Gtk.main_quit)
> Do you still use this main block for testing?

Yes, this should more or less work, perhaps requires some
paths tweaking. I'll look at it some time.



More information about the anaconda-patches mailing list