> core: refactor update_slave_connection() as virtual function of master device

Can you remove the "as" from "nm_device_as_master_update_slave_connection()" and its virtual method? It seems weird with that in the name.

I would have the NMSettingConnection-updating part of that code be in the default implementation of the method in NMDevice, with NMDeviceBridge and NMDeviceTeam calling down to it, and NMDeviceBond just not implementing the vmethod at all and getting only the default implementation.

>- * nm_bridge_update_slave_connection:
>+ * as_master_update_slave_connection:

Move the docs to the wrapper function definition in nm-device.c.

>-			nm_log_dbg (LOGD_DEVICE, "(%s): cannot generate connection for slave before its master (%s)",
>-			            ifname, nm_platform_link_get_name (master_ifindex));

why did you move this code?

>+			nm_log_dbg (LOGD_DEVICE, "(%s): master device '%s' failed to update slave connection: %s",

The cases where the subclasses return errors seem like they should be logged at greater than debug.

>+	g_set_error (error,
>+	             NM_DEVICE_ERROR,
>+	             NM_DEVICE_ERROR_INVALID_DEVICE_TYPE,
>+	             "master device '%s' cannot update a slave connection for slave device '%s'",
>+	             iface, nm_device_get_iface (slave));
>+	g_return_val_if_reached (FALSE);

Don't both set a GError AND return-if-reached. Either this is a bug in NetworkManager (in which case don't set a GError), or it's not (in which case don't return-if-reached).

In this case, this is not a bug in NM; the code would get hit if the kernel added a new master type that NM didn't know about, and the user created one of those, with a slave, from outside NM. NM would see the master device as NMDeviceGeneric, but would then fail to be able to update the slave from it.

(So, "INVALID_DEVICE_TYPE" doesn't seem like the right name for that error.)


A few commits later you remove the '#include "nm-device-team.h"' from nm-device.c, but I think you can actually remove all three master-type includes as part of this commit?



> core: create virtual devices via NMDeviceFactory

That makes it sound like we're going to create *all* virtual devices via factories. "core: allow creating virtual devices via NMDeviceFactory"

>+					nm_log_dbg (LOGD_DEVICE, "(%s) failed to create virtual device: %s",

again this seems like it should be more than debug

>+					g_error_free (error);

It's generally best to always use g_clear_error() if you aren't immediately returning (or otherwise leaving the scope where the GError is defined), to ensure that it's NULL again if other code later in the function tries to use it.

(In reply to comment #1)
> > core: refactor update_slave_connection() as virtual function of master device
> 
> Can you remove the "as" from "nm_device_as_master_update_slave_connection()"
> and its virtual method? It seems weird with that in the name.

Done


> I would have the NMSettingConnection-updating part of that code be in the
> default implementation of the method in NMDevice, with NMDeviceBridge and
> NMDeviceTeam calling down to it, and NMDeviceBond just not implementing the
> vmethod at all and getting only the default implementation.

Not done. The derived class also sets the "slave-type" of NMSettingConnection. The parent class does not know that easily...

Possible solutions for that seem inferior to me:
 - add a out parameter "const char *out_slave_type" to the virtual 
   master_update_slave_connection().
 - hard code the 3 known slave types in 
   nm_device_master_update_slave_connection()
 - add nm_device_get_setting_type() to NM core.


> >- * nm_bridge_update_slave_connection:
> >+ * as_master_update_slave_connection:
> 
> Move the docs to the wrapper function definition in nm-device.c.

Done.


> 
> >-			nm_log_dbg (LOGD_DEVICE, "(%s): cannot generate connection for slave before its master (%s)",
> >-			            ifname, nm_platform_link_get_name (master_ifindex));
> 
> why did you move this code?

dcbw preferred that the device does not call up to the manager (in search of the master device). Instead the manager should provide the master device when calling nm_device_generate_connection()


> >+			nm_log_dbg (LOGD_DEVICE, "(%s): master device '%s' failed to update slave connection: %s",
> 
> The cases where the subclasses return errors seem like they should be logged at
> greater than debug.

Done. Logged as ERR.


> >+	g_set_error (error,
> >+	             NM_DEVICE_ERROR,
> >+	             NM_DEVICE_ERROR_INVALID_DEVICE_TYPE,
> >+	             "master device '%s' cannot update a slave connection for slave device '%s'",
> >+	             iface, nm_device_get_iface (slave));
> >+	g_return_val_if_reached (FALSE);
> 
> Don't both set a GError AND return-if-reached. Either this is a bug in
> NetworkManager (in which case don't set a GError), or it's not (in which case
> don't return-if-reached).
>
> In this case, this is not a bug in NM; the code would get hit if the kernel
> added a new master type that NM didn't know about, and the user created one of
> those, with a slave, from outside NM. NM would see the master device as
> NMDeviceGeneric, but would then fail to be able to update the slave from it.
> 
> (So, "INVALID_DEVICE_TYPE" doesn't seem like the right name for that error.)

Done. Renamed to NM_DEVICE_ERROR_UNSUPPORTED_DEVICE_TYPE. Probably the reason is, that you did not install the team-plugin.


> A few commits later you remove the '#include "nm-device-team.h"' from
> nm-device.c, but I think you can actually remove all three master-type includes
> as part of this commit?

Done.


> > core: create virtual devices via NMDeviceFactory
> 
> That makes it sound like we're going to create *all* virtual devices via
> factories. "core: allow creating virtual devices via NMDeviceFactory"
> 
> >+					nm_log_dbg (LOGD_DEVICE, "(%s) failed to create virtual device: %s",
> 
> again this seems like it should be more than debug
> 
> >+					g_error_free (error);
> 
> It's generally best to always use g_clear_error() if you aren't immediately
> returning (or otherwise leaving the scope where the GError is defined), to
> ensure that it's NULL again if other code later in the function tries to use
> it.

Both done.


Repushed branch (doing some new testing)

Rebased on master again (resolving conflicts).

Please especially look at
 >> team/trivial: rename NM_TEAM_ERROR* to NM_DEVICE_TEAM_ERROR*
Is this an API change and are those names exported via DBUS?

looks good (other than the error API question, which I still defer to dcbw on).

one nitpick:

> core: refactor update_slave_connection() as virtual function of master device

>-	g_return_val_if_fail (NM_IS_DEVICE (slave), FALSE);
>-	g_return_val_if_fail (NM_IS_CONNECTION (connection), FALSE);
>+	g_return_val_if_fail (NM_IS_DEVICE_BRIDGE (self), FALSE);
>+	g_return_val_if_fail (ifindex_slave > 0, FALSE);

If you're checking NM_IS_DEVICE(self) in the wrapper function, you don't need to check NM_IS_DEVICE_BRIDGE(self) in the subclass implementation. We can trust gobject to get it right.

>+	g_return_val_if_fail (self, FALSE);
>+	g_return_val_if_fail (slave, FALSE);
>+	g_return_val_if_fail (connection, FALSE);

ah, well, ok, you're not checking NM_IS_DEVICE there, but you should. :)

(In reply to comment #4)
> looks good (other than the error API question, which I still defer to dcbw on).
> 
> one nitpick:

Done.


Rebased on master. Added two new fixup! commits

> core: refactor update_slave_connection() as virtual function of master device

Extra whitespace in nm-manager.c::get_existing_connection()


(In reply to comment #3)
> Rebased on master again (resolving conflicts).
> 
> Please especially look at
>  >> team/trivial: rename NM_TEAM_ERROR* to NM_DEVICE_TEAM_ERROR*
> Is this an API change and are those names exported via DBUS?

Yes, that's a D-Bus exported error name, but you've only changed the internal names.  The only 2 things dbus-glib cares about are the GEnumValue "value_nick" (which you didn't change, because it's the /*< nick=ConnectionNotTeam >*/ piece) and the interface passed to dbus_g_error_domain_register() which you didn't change.

(aside: our usage of dbus_g_error_domain_register() is kinda screwed up, since we don't pass an interface-name in most places.  That's a historical mistake but we can't really change it now since it's our D-Bus API)

So TLDR; I think we're OK because the error nicknames haven't changed.

(In reply to comment #6)
> 
> So TLDR; I think we're OK because the error nicknames haven't changed.

Thanks Dan, for the explaination.


Merged to master as http://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/?id=918ff2b0f7df10afe8354291634c8abc965dda0a