GNOME Bugzilla – Bug 708245
[review] jklimes/team: applet/editor support for Team devices
Last modified: 2013-10-11 07:21:06 UTC
Please review the branch jklimes/team. It contains patches from jpirko: https://mail.gnome.org/archives/networkmanager-list/2013-September/msg00027.html and I have added one patch to enhance JSON config widget.
Created attachment 255156 [details] [review] Change how to choose the team_num for a new team device This algorithm used in page-team.c is the same as for page-bond.c, page-bridge.c, and possibly others. Maybe choose it there too?
s/, and possibly others//g nope, just bond, bridge and team AFAIS.
The patch from comment #1 looks fine. I just reverted the part creating the setting, it didn't compile. I added the fixed patch to jklimes/team branch and rebased to master. Thomas, please change the interface num selection also for bond and bridge and push.
I have rebased jklimes/team to master and added two commits: 25a6d1e editor: only allow adding slaves of the same type to master Team 97a4934 editor: change JSON TeamPort config widget from GEntry to GTextView Please review so that it can be pushed soon.
(In reply to comment #4) > 25a6d1e editor: only allow adding slaves of the same type to master Team Not quite; it's "of the same arp type" really. So you can mix Ethernet and Wi-Fi, you just can't mix either of those with InfiniBand. (The bond page doesn't run into this because it doesn't support Wi-Fi.) > 97a4934 editor: change JSON TeamPort config widget from GEntry to GTextView Hm... the UI there (and on the Team page) makes it look like it's going to store the filename in the ifcfg file, and load the JSON data from that file at runtime. (People get confused about that with the X509 certificate selector buttons.) I think you should have just an "Import..." button (not a filechooser button), that pops up a filechooser, so that the filename of the file never shows in the editor window.
(In reply to comment #5) > (In reply to comment #4) > > 25a6d1e editor: only allow adding slaves of the same type to master Team > > Not quite; it's "of the same arp type" really. So you can mix Ethernet and > Wi-Fi, you just can't mix either of those with InfiniBand. (The bond page > doesn't run into this because it doesn't support Wi-Fi.) > Updated to support either InfiniBand or (ethernet, Wi-Fi, VLAN) > > 97a4934 editor: change JSON TeamPort config widget from GEntry to GTextView > > Hm... the UI there (and on the Team page) makes it look like it's going to > store the filename in the ifcfg file, and load the JSON data from that file at > runtime. (People get confused about that with the X509 certificate selector > buttons.) I think you should have just an "Import..." button (not a filechooser > button), that pops up a filechooser, so that the filename of the file never > shows in the editor window. Done. I squashed the following commits into one. editor: change JSON TeamPort config widget from GEntry to GTextViewconnection-editor: some .ui file layout fixes for Team and Team Port editor: change JSON Team config widget from GEntry to GTextView Please re-review top two commits of jklimes/team.
looks good
Merged jklimes/team to master: ba7652f team: merge a branch adding support for Team to applet/editor (bgo #708245) e98d975 editor: only allow adding compatible slaves to master Team connection 697f7f4 editor: change JSON Team/TeamPort config widget from GEntry to GTextView 148d15a team: make algorithm for choosing new team_num deterministic e2bf4cd libnm-gtk: recognize Team as a valid virtual device 9ce2701 editor: use unambiguous mnemonic kyes for Team and TeamPort pages 6ecb131 add support for team devices
Two things I saw in "editor: change JSON Team/TeamPort config widget from GEntry to GTextView"... 1) the import dialog should be cached and torn down in both the port and the main team page, otherwise you can click out of the import dialog, hit "Cancel" and the editor will crash with a use-after-free because the page is disposed but the response handler for the dialog still tries to use it 2) the import dialog's parent should probably be set to the page's window parent (with gtk_widget_get_parent_window() maybe?) so that it gets attached to the parent window, because it really is modal to the parent. This would also let the shell draw it as a sheet instead of just layering it over the team tab
(In reply to comment #9) > Two things I saw in "editor: change JSON Team/TeamPort config widget from > GEntry to GTextView"... > > 1) the import dialog should be cached and torn down in both the port and the > main team page, otherwise you can click out of the import dialog, hit "Cancel" > and the editor will crash with a use-after-free because the page is disposed > but the response handler for the dialog still tries to use it > Well, so make the import dialog really modal by calling gtk_dialog_run() instead of "response" signal. This will prevent users from cancelling the main page. > 2) the import dialog's parent should probably be set to the page's window > parent (with gtk_widget_get_parent_window() maybe?) so that it gets attached to > the parent window, because it really is modal to the parent. This would also > let the shell draw it as a sheet instead of just layering it over the team tab Parent window set for the dialog. See updated jklimes/team branch.
Only one comment about the patch, formatting: + if (gtk_dialog_run (GTK_DIALOG (dialog)) == GTK_RESPONSE_ACCEPT) + { Also pushed a quick fixup patch for getting toplevel for team master. The json widget won't necessarily have a toplevel parent yet at page init time, we have to wait until the widget is realized (and thus embedded in a window) to get it. Please review that one, and feel free to squash it into yours when you merge. Rest looks good, thanks!
Thanks for reviewing. Fixed, squashed, pushed to master: d43f58da694cd16f274eef19c5dde9f7d4b8ca32