Re: API changes

["Tambet Ingo" <tambet gmail com>]

On Thu, Mar 13, 2008 at 9:29 AM, Will Stephenson <wstephenson kde org> wrote:
>  org.freedesktop.NetworkManager
>  Methods:
>  #1
>  ActivateDevice ( o: device, s: service_name, o: connection, o:
>  specific_object ) → nothing
>
>  suggestion: move to org.freedesktop.NetworkManager.Device.Activate(s:
>  service_name, o: connection, o: specific_object ) → nothing
>
>  rationale: Helmut told me that this had already been discussed, and that
>  because you could imagine Connection.Activate( Device ) or Device.Activate(
>  Connection) it was decided to put it on the manager.  If this was
>  communicated properly,  this is a worst of all compromise.  Since Connection
>  is on the settings service, the action can't be carried out there, so stay
>  object-oriented and move this to Device.
>
>  As I'm wrapping NM in Solid for KDE 4, I'll probably move this method onto the
>  Device class anyway, but moving it in NM would be better for both projects
>  IMO.

It was never a discussion between Connection.Activate(Device) vs
Device.Activate(Connection). The current solution isn't a compromise
either, it's just the way things have to be right now. NMManager is
the thing that has access to available settings and devices, hence
it's the thing that can tie them together for activation.
Device.Activate(connection_path) is not possible since device can't
translate that connection_path to Connection without the help of
NMManager. It can't do that since NMManager has to contain the list of
devices, and circular dependencies are much bigger OOP offenders than
what you suggest. The reason why it was pretty much impossible to add
new device types to 0.6 branch was because of no class hierarchy,
everything just called everything and to add a new device type meant
changing almost all source files. With the strict hierarchy (that I've
been defending very hard, and still do), adding a new device means
implementing a subclass of NMDevice, a subclass of NMSetting and a
registration function in NMHalManager. I'm not willing to give up all
this just to have Device.Activate(Connection).

>  #2
>   errors:
>     org.freedesktop.NetworkManager.Error.ConnectionActivating
>     Another connection is already activating or the same connection is already
>  active.
>
>  org.freedesktop.NetworkManager.Error.ConnectionActivating
>
>  Does this mean the same connection is already activating or is another
>  connection activating on this device?
>
>  Suggestion: split into
>  org.freedesktop.NetworkManager.Error.ConnectionAlreadyActive
>  org.freedesktop.NetworkManager.Error.ConnectionActivating
>
>  Although I'm not sure when this error would be raised, since a client might
>  want to interrupt a connection attempt (maybe started with bad parameter,
>  maybe it's going wrong and failing to error).
>
>  #3
>  Sleep ( b: sleep ) → nothing
>   Control the NetworkManager daemon's sleep state. When asleep, all interfaces
>  that it manages are deactivated.
>  Parameters
>  sleep - b
>   Indicates whether the NetworkManager daemon should sleep or wake.
>
>  suggestion: It's not clear from the method name what this does
>  SetSleepState( b: asleep ) perhaps?

It would be even clearer to rename it to EnableNetworking(b: enabled).

>  #4
>  enum:
>  NM_STATE_CONNECTING = 2
>   The NetworkManager daemon is connecting a device.
>  What does this mean when one device is already active and another is
>  connecting?

The NMState enum is not 100% accurate in case of multiple devices and
it isn't meant to. It reflects the "best" state of NM. The reason
behind it is to give easier API for applications that only care if the
machine has network connection or not. So for your specific question,
if a device is active and another one is activating, the NMState
reamains NM_STATE_CONNECTED. If an app wants more information, it can
always get NMManager, it's list of devices and the state of each
device.

>  org.freedesktop.NetworkManager.AccessPoint
>  #5
>
>  roperties:
>  HwAddress - s - (read)
>  The hardware address of the access point.
>  suggestion: make this uint64 type="t"?  IP addresses are uints, and uint64
>  fits both 48 bit and 64 bit MAC addresses, and is smaller than the unicode
>  string needed for a macaddress.

The data type for it in the kernel is "struct ether_addr", just
because it fits in a uint64 doesn't mean it's a good data type for it.
There's no calculations to perform on it, the only useful operation on
it would be to print it out. Then why bother with the conversions to
save a few bites on communication when you're going to waste these
same bites when you're actually going to do  anything (display) it?

>  org.freedesktop.NetworkManager.Device.Wired
>  Properties:
>  #6
>  HwAddress - s - (read)
>   Hardware address of the device.
>  Suggestion: change to 't' as above

Just as above.

>  #7
>  org.freedesktop.NetworkManagerSettings
>  Signals:
>  NewConnection ( o: connection )
>  Suggestion: rename to ConnectionAdded to be consistent with
>  NetworkManager.DeviceAdded

Sounds good to me.

>  #8
>  Where is ConnectionRemoved on this interface?  See comment #11 below.
>
>
>  Methods:
>  #9
>  ListConnections ( ) → ao
>  Suggestion: rename to GetConnections to be consistent with other Get list type
>  methods

Agreed.

>  org.freedesktop.NetworkManagerSettings.Connection
>  #10
>  Suggestion: for the settings maps, switch to u instead of s keys defined by
>  well-known enums - faster, smaller, safer

Setting types are not necessarily well known. I've been trying to keep
NM as extensible as possible and currently the only thing missing in
NM to allow plugins (to implement new devices as I already described)
is a module loader. There are already hooks to register new setting
and device types. The first place we're going to need it is probably
for gsm/cdma devices which all have their own proprietary protocols
and it would get insane pretty quickly to try to do it all in once
class.

>  Signals:
>  #11
>  Removed ( )
>  Emitted when this settings object was removed.
>
>  IIUC this is emitted before the object is removed, or else DBus would likely
>  have a fit.  If this is the case, then it has different semantics to
>  NetworkManager.DeviceRemoved(o).  If prior to removal, should be called
>  AboutToBeRemoved.  Clarify if what happens to this object after it emits this
>  signal.
>
>  Suggestion: move to NetworkManagerSettings.ConnectionRemoved to be emitted
>  after removal
>
>  org.freedesktop.NetworkManager.Device
>  Properties:
>  #12
>  Ip4Address - i - (read)
>   The IPv4 address actually bound to the device.
>  FIXME: wrong type - should be u.

This should be removed instead, there's already NMIP4Config property
that includes the ip address.

>  #13
>  FIXME: devices can have >1 IP address so make this 'au' with the canonical
>  address first, OR state that secondary IP addresses are listed in the
>  Ip4Config.
>  #14
>  Ip4Config - o - (read)
>  FIXME: make clear that is is the data used to configure the device, not the
>  actual configuration
>  QUESTION: since in the case of a dhcp IPv4 configuration, the referenced
>  Ip4Config from the Settings will know nothing about the actual IP address,
>  netmask etc, so where are these available apart from the Ip4Address above?

Agreed, this is going to change when someone gets around to implement
it. Until then, it'll just make it harder to use for no gain.

>  #15
>  HwAddress - s - (read)
>   The hardware address of the device.
>  suggestion: uint64 (type="t") as above

As above.

>  #16
>  org.freedesktop.NetworkManager.IP4Config
>  Address - u - (read)
>  IPv4 address.
>  Suggestion: what about multiple addresses?change to 'au'

Same as my comment to #13.

>  #17
>  Hostname - s - (read)
>  The hostname associated with this IPv4 address.
>  Suggestion: what about multiple hostnames? change to 'as' with canonical
>  hostname first

There can be one hostname for a system and multiple ip addresses. What
you suggest makes no sense.

>  org.freedesktop.NetworkManager.VPN.Manager
>  ListConnections ( ) → ao
>  #18
>   Get the list of active VPN connections.
>  suggestion: rename to GetConnections for consistency with other get-list type
>  methods

Agreed.

>  errors:
>  #19
>  org.freedesktop.NetworkManager.VPN.Error.BadArguments
>  org.freedesktop.NetworkManager.VPN.Error.WrongState
>  suggestion: too general to be useful - should have discrete specific errors to
>  describe the problem - examples?

These are not the errors you want to show to the user. If these
happen, the VPN plugin is doing something wrong and there's no way to
recover from it other than fix the VPN plugin code. They're specific
enough for that.

>  org.freedesktop.NetworkManager.VPN.Plugin
>  method:
>  #20
>  NeedSecrets (a{sa{sv}}) -> s
>  Suggestion: sounds like a signal. Rename to
>  IdentifySecretSettingsForConnection.

I didn't invent that name but I prefer NeedSecrets.

>  NM_VPN_CONNECTION_STATE enum:
>  #21
>  NM_VPN_CONNECTION_STATE_CONNECT = 3
>   The VPN connection is being established.
>  suggestion: Should be CONNECTING so not confused with an action.
>
>  #22
>  NM_VPN_CONNECTION_STATE_IP_CONFIG_GET = 4
>  suggestion:
>   The VPN connection is getting an IP address. FIXME: Should be
>  NM_VPN_CONNECTION_STATE_IP_CONFIG_GETTING so isn't confused with an action

I don't have any opinion on these two.

Tambet