GNOME Bugzilla – Bug 771190
lr/async-client: client should use the async API to obtain the NMClient instance
Last modified: 2016-11-21 11:57:23 UTC
Otherwise it synchronously initializes the objects one by one, which tends to get really slow. Work in progress: https://cgit.freedesktop.org/NetworkManager/NetworkManager/log/?h=lr/ac
Ready for review: https://cgit.freedesktop.org/NetworkManager/NetworkManager/log/?h=lr/async-client
>> cli: make it possible to call sub-commands with client obtained asynchronously there are still "return NMC_RESULT_SUCCESS;" lines in process_command_line(). Can you fix the whitespace errors in process_command_line() »···»··· »··»···g_string_printf (nmc ^^^^ + } else { + nmc->return_value = cmd->func (nmc, argc, argv); + } no braces? ^ + if (process_command_line (&nm_cli, argc, argv)) + g_main_loop_run (loop); it's looks strange to me to parse the command line multiple times. The only case process_command_line() returns TRUE is with »···/* Now run the requested command */ »···nmc_do_cmd (nmc, nmcli_cmds, *argv, argc, argv); »···return TRUE; I think the iteration of the mainloop should go inside nmc_do_cmd().
+ g_object_set_data_full (G_OBJECT (object), "nm-object", + nm_object, g_object_unref); g_object_set_data() requires to internalize the string each time. Can you instead use g_object_set_qdata_full() and allocating a quark with G_DEFINE_QUARK(). - g_clear_pointer (&priv->hostname, g_free); + + /* Clear properties */ + if (priv->hostname) { + g_free (priv->hostname); + priv->hostname = NULL; + } doesn't seem like an improvement. - _nm_dbus_new_connection_async (cancellable, init_async_got_bus, init_data); + interfaces = g_dbus_object_get_interfaces (priv->object); + for (prop = props; *prop; prop++) { + val = g_dbus_proxy_get_cached_property (proxy, *prop); wrong indention. g_assert (priv->object && priv->object_manager); maybe just g_return_val_if_fail()? Assertion failures are painful.
I see a strange behavior with these patches: 'nmcli c' gives sometimes (one time every ~10) a wrong output with only active connections, and they are marked as invisible: NAME UUID TYPE DEVICE <invisible> br1-static 89303bf1-ad17-4bd0-985d-1bdf42fa59a2 bridge br1 <invisible> docker0 995b1f7e-2856-443b-8f76-39af20f2463c bridge docker0 <invisible> enp0s25-dhcp 9734ce12-91c6-409f-bcc2-5139f5127bd2 802-3-ethernet enp0s25 NetworkManager logs don't show any error. > cli: get rid of client-global connections list + for (i = 0; i < connections->len; i++) { + NMConnection *con = NM_CONNECTION (connections->pdata[i]); const char *id = nm_connection_get_id (con); - connections[i] = id; + connection_names[i] = id; } Maybe drop the temporary variable @id? > cli: make it possible to call sub-commands with client obtained asynchronously nmc_init (&nm_cli); - g_idle_add (start, &args_info); args_info is no longer used in this function, can you remove it? + } else { + nmc->return_value = cmd->func (nmc, argc, argv); + } + g_simple_async_result_complete_in_idle (simple); I think @simple should be unreferenced after each call to complete{_in_idle}().
> cli: get rid of client-global connections list The commit log seems wrong, I don't think the commit has any effect on when the remote connections get loaded, since nm_client_get_connections() just calls nm_remote_settings_get_connections() which just returns the internal pointer... > libnm/nm-manager: don't block the object creation on permissions There is nm_manager_get_permission_result() that would use the permissions reply, but I think your approach here is OK. We should just somehow block nm_manager_get_permission_result() until we get the first permissions reply back, maybe? That might be a bit tricky...
(In reply to Thomas Haller from comment #2) > >> cli: make it possible to call sub-commands with client obtained asynchronously > > there are still "return NMC_RESULT_SUCCESS;" lines in process_command_line(). > > > > Can you fix the whitespace errors in process_command_line() > > »···»··· »··»···g_string_printf (nmc > ^^^^ > > > > + } else { > + nmc->return_value = cmd->func (nmc, argc, argv); > + } > > no braces? ^ I sort of prefer them, because it makes the printf debugging easier and it seems a bit weird to me visually if one branch of the conditional has it, but not the other on. Dropped them, anyway. > + if (process_command_line (&nm_cli, argc, argv)) > + g_main_loop_run (loop); > > it's looks strange to me to parse the command line multiple times. The only > case process_command_line() returns TRUE is with > > »···/* Now run the requested command */ > »···nmc_do_cmd (nmc, nmcli_cmds, *argv, argc, argv); > »···return TRUE; > > I think the iteration of the mainloop should go inside nmc_do_cmd(). There's no loop involved. (In reply to Thomas Haller from comment #3) (this one is to a different review) (In reply to Beniamino Galvani from comment #4) > I see a strange behavior with these patches: 'nmcli c' gives sometimes > (one time every ~10) a wrong output with only active connections, and > they are marked as invisible: > > NAME UUID TYPE > DEVICE > <invisible> br1-static 89303bf1-ad17-4bd0-985d-1bdf42fa59a2 bridge > br1 > <invisible> docker0 995b1f7e-2856-443b-8f76-39af20f2463c bridge > docker0 > <invisible> enp0s25-dhcp 9734ce12-91c6-409f-bcc2-5139f5127bd2 > 802-3-ethernet enp0s25 > > NetworkManager logs don't show any error. I think this is a bug that's fixed in the object-manager branch. (This is a reminder to merge this only *after* that branch is merged.) > > cli: get rid of client-global connections list > > + for (i = 0; i < connections->len; i++) { > + NMConnection *con = NM_CONNECTION (connections->pdata[i]); > const char *id = nm_connection_get_id (con); > - connections[i] = id; > + connection_names[i] = id; > } > > Maybe drop the temporary variable @id? Done. > > cli: make it possible to call sub-commands with client obtained asynchronously > > nmc_init (&nm_cli); > - g_idle_add (start, &args_info); > > args_info is no longer used in this function, can you remove it? Removed. > + } else { > + nmc->return_value = cmd->func (nmc, argc, argv); > + } > + g_simple_async_result_complete_in_idle (simple); > > I think @simple should be unreferenced after each call to > complete{_in_idle}(). You're right. Fixed. (In reply to Dan Williams from comment #5) > > cli: get rid of client-global connections list > > The commit log seems wrong, I don't think the commit has any effect on when > the remote connections get loaded, since nm_client_get_connections() just > calls nm_remote_settings_get_connections() which just returns the internal > pointer... Tried to improve the wording. > > libnm/nm-manager: don't block the object creation on permissions > > There is nm_manager_get_permission_result() that would use the permissions > reply, but I think your approach here is OK. We should just somehow block > nm_manager_get_permission_result() until we get the first permissions reply > back, maybe? That might be a bit tricky... Oh, how could I miss this one. In any case, if we keep this patch as is, the applet will start with "Create {hidden,} wi-fi" greyed-out, but will quickly catch up anyway. Same about the conenction editor and the GNOME control center. I guess that's good enough? nmcli uses it as well. Maybe we could just make is wait for the first round of permission-chanaged signals? What do you think? (Pushed an updated branch)
(In reply to Lubomir Rintel from comment #6) > nmcli uses it as well. Maybe we could just make is wait for the first round > of permission-chanaged signals? What do you think? https://github.com/NetworkManager/NetworkManager/commit/df87fea4896707028aab17ad4c4bd6202cb446ac (a github link, as fdo seems to be having issues atm)
>> libnm/nm-manager: don't block the object creation on permissions the call to nmdbus_manager_call_get_permissions() should not take a reference. Instead, it should make sure that init_data->cancellable is non-NULL (and if it is, create one, so that we always have a cancellable), and then in the callback check for cancelled first. + /* The client didn't get the permissions reply yet. Subscribe to changes. */ + g_sig indention rest lgtm. (not tested yet)
Merged in f5c5565d3ff77674d37427d4dccd19c67c15ab4e