> editor: add connection argument to the new page function

+       id = nm_setting_connection_get_id (s_con);
+       if (!id) {
+               connections = nm_client_get_connections (client);
+               id = ce_page_get_next_available_name (connections, format);

Leaks id.

The rest LGTM.

+         g_object_set_data (G_OBJECT (list), "nm-connection-editor-connection",
+                            connection);

set_data_full() and nm_object_ref(connection). It's not clear that the connection instance stays alive long enough.






why does mobile_connection_new() require no connection:
+    g_return_if_fail (!connection);
but most other implementations do:
+    if (!connection)
+         connection = nm_simple_connection_new ();






 vlan_connection_new (GtkWindow *parent,
                      const char *detail,
                      gpointer detail_data,
+                     NMConnection *connection,
     ....
+    if (!connection)
+         connection = nm_simple_connection_new ();
+    ce_page_complete_connection (connection,

why doesn't this leak connection? Why not just require the caller to pass in a connection, always?

(In reply to Beniamino Galvani from comment #1)
> > editor: add connection argument to the new page function
> 
> +       id = nm_setting_connection_get_id (s_con);
> +       if (!id) {
> +               connections = nm_client_get_connections (client);
> +               id = ce_page_get_next_available_name (connections, format);
> 
> Leaks id.

Added a fixup.

(In reply to Thomas Haller from comment #2)
> +         g_object_set_data (G_OBJECT (list),
> "nm-connection-editor-connection",
> +                            connection);
> 
> set_data_full() and nm_object_ref(connection). It's not clear that the
> connection instance stays alive long enough.

Okay. Added a fixup.

> why does mobile_connection_new() require no connection:
> +    g_return_if_fail (!connection);
> but most other implementations do:
> +    if (!connection)
> +         connection = nm_simple_connection_new ();

Well, none of the connections technially do at this time, since we only support VPN connection import. I was too lazy to make the mobile & bluetooth accept connection. I did it now; added a fixup.

>  vlan_connection_new (GtkWindow *parent,
>                       const char *detail,
>                       gpointer detail_data,
> +                     NMConnection *connection,
>      ....
> +    if (!connection)
> +         connection = nm_simple_connection_new ();
> +    ce_page_complete_connection (connection,
> 
> why doesn't this leak connection?

result_func takes the ownership. I didn't change it.

> Why not just require the caller to pass in
> a connection, always?

Because there's one new_func that always creates the connection itself (that is the vpn_connection_import()).

Pushed fixups.

(In reply to Lubomir Rintel from comment #3)
> (In reply to Beniamino Galvani from comment #1)

> > why doesn't this leak connection?
> 
> result_func takes the ownership. I didn't change it.

which would mean that the new argument connection transfers ownership all the way from

 - nm_connection_list_create()
   - new_connection_of_type()
     - new_func()

which is quite non-obvious... and in fact there are at least two bugs there:


1)
+    NMConnection *connection = g_object_get_data (G_OBJECT (list), "nm-connection-editor-connection");
 
+    nm_connection_list_create (list, ctype, detail, connection);

if you really transfer ownership, the it needs either g_object_steal_data() or g_object_ref(connection).



2)

nm_connection_list_create (NMConnectionList *self, GType ctype, const char *detail, NMConnection *connection)

   if (!types[i].name) {

        /* leaks @connection */

   else  {

        /* pass on ownership of @connection */
        new_connection_of_type (GTK_WINDOW (self->dialog),
                                  detail,
                                  NULL,
                                  connection,
   }



let's not introduce such a complicated semantic of passing ownership though several layers, apparently it is error-prone.

+         connection = vpn_connection_from_file (import);
+         g_idle_add (idle_create_connection, list);
+         g_object_set_data (G_OBJECT (list), "nm-connection-editor-ctype",
+                            GSIZE_TO_POINTER (NM_TYPE_SETTING_VPN));
+         g_object_set_data_full (G_OBJECT (list), "nm-connection-editor-connection",
+                                 nm_g_object_ref (connection), nm_g_object_unref);


this also leaks @connection

Thomas, does this look more sensible?

https://git.gnome.org/browse/network-manager-applet/commit/?h=lr/import&id=c97b00d60

Pushed a few more fixups.

Notably indeed got rid of the "new_func can create a connection" semantics.

Fixed in 2ee9f80e3f65370be801a6e0cfaa4f69202b8c69