Re: API changes

[Dan Williams <dcbw redhat com>]

On Thu, 2008-03-13 at 16:29 +0100, Will Stephenson wrote:
> On Thursday 13 March 2008, Dan Williams said:
> > A few API changes, both in the Settings specification, and in D-Bus.
> > Some of these are thanks to Will's documentation effort and mini-review
> > of the API while he was doing that.
> 
> Great, glad to see the docu work is helping.
> 
> And here's a bunch more API question marks that Helmut and myself came up 
> with:
> 
> They are organised by interface, and numbered, just grep for the #'s.
> 
> Will
> 
> 
> --------------------------------
> 
> 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

This is somewhat hard internally, since the NMDevice objects are
children of the NMManager object, and therefore have no access to the
connection list or the settings services.

I'm leaning more towards renaming this to something like:

ActivateConnection( o: connection, o: device ...)

since we're not going to do connections spanning multiple devices for
0.7 (bridging, etc).

> 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.
> 
> #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

Probably a good thing to do.

> 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?

Well, Tambet and I figured it was kind of pointless to have two
different functions.  We could make them properties of the manager
object and use the D-Bus property interface, but they are easier to lock
down when they are methods.  I guess I agree that the current name is a
bit confusing; not sure quite what to call it though.

> #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?

Note that we need to preserve this state enum for minimal backwards
compatibility with 0.6 clients.  However, the mapping could be better in
the manager right now.

STATE_CONNECTED: there is at least one active connection
STATE_CONNECTING: there are no active connections, but at least one
connection is activating on a device
STATE_DISCONNECTED: there are no active connections

> 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.

When are 64-bit MAC addresses going to show up?  That would take a
pretty drastic change in the 802.11 protocol, so I don't think we need
to care about it right now.

WRT to this and the other comments about HwAddress; it's a device-type
specific UID, it just happens that both 802.11 (WiFi), 802.16
(Bluetooth), and 802.3 (Ethernet) all use 48-bit MAC addresses.  But I'd
imagine we'll use HwAddress for the IMEI on GSM cards, the ESN/MEID on
CDMA cards, etc.

Perhaps it's confusing that the property is the same name for all the
different device types even though the contents would be different?  I
think it would be worse if the type was also consistent across all
devices.  One benefit of keeping it as a string is that we're open to
better forwards compatibility; we don't have to break API when something
happens like CDMA changing from 32-bit ESN to 56-bit MEID happens.  The
downside of course is that the client has to parse the item; but clients
will have to do some amount of parsing anyway.  At least we have quite
useful library functions for ether_ntop()/ether_pton().

There's an argument to be made that the name should stay "HwAddress" but
the type should change to ay[6] (for IEEE MAC addresses) or ay[7] (CDMA
56-bit MEID) or whatever, with a length specific for the device type.

> org.freedesktop.NetworkManager.Device.Wired
> Properties:
> #6
> HwAddress - s - (read)
>  Hardware address of the device. 
> Suggestion: change to 't' as above
> 
> #7
> org.freedesktop.NetworkManagerSettings
> Signals:
> NewConnection ( o: connection )
> Suggestion: rename to ConnectionAdded to be consistent with 
> NetworkManager.DeviceAdded

Probably.

> #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

Ok.

> 
> 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

Probably not going to happen...  I used strings as the key names for a
few reasons.  First that it's easier client-side when storing the config
info.  Most config stores are text-based; with the current system you
don't have to have a bit translation table or anything to do enum ->
setting name or enum -> key.  I realize you can't do a bit switch
somewhere on the key names, but the usage of hash tables or maps should
mitigate this on the client.  It's also a lot easier to debug
underneath, and if any of this is going through a really performance
critical path, we've already got problems.

> 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

I remember trying to decide between these two models.  In the end, it's
less object-oriented to have the removal signal be on the parent of the
object rather than the object itself (Removed is essentially the "I'm
now dead" signal), but it is a more "balanced" API to have both
added/removed on the parent.  I don't really know if this makes a real
difference or not, because I've heard arguments for both patterns.  I'm
sort of ambiguous about this, we should pick pattern and stick with it
rather than, as you point out, the mishmash we've got today.

> org.freedesktop.NetworkManager.Device
> Properties:
> #12
> Ip4Address - i - (read)
>  The IPv4 address actually bound to the device. 
> FIXME: wrong type - should be u.

Looks like a simple error, should fix it.

> #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.

I think the Ip4Address on the device item should just go away.

> #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?

Wrong actually; it should be the real IP4 config applied to the device,
which is a composite of both the connection's ip4 setting, and any
automatic configuration of the device.  So this object should represent
the actual configuration of the object.

I would like to expose the DHCP-returned options as a separate interface
of the NMDevice or whatever with it's own PropertyChanged signals.
That's the full replacement for the functionality provided by dhcdbd,
which clients could query for all the DHCP options.  It's highly
unlikely that NM will _ever_ poke NIS or WINS or whatever, so this would
be the method that clients would use to update their local NIS or WINS
information when the connection changed.

> #15
> HwAddress - s - (read)
>  The hardware address of the device. 
> suggestion: uint64 (type="t") as above
> 
> #16
> org.freedesktop.NetworkManager.IP4Config
> Address - u - (read)
> IPv4 address. 
> Suggestion: what about multiple addresses?change to 'au'

We should change this to the [auu(u)] format that the ip4 setting uses.

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

Hostnames and DNS searches are somewhat unrelated to IP4 settings
though, so I'd like to figure out what to do here.  These also apply to
IPv6 as well.

Also, I don't think we care about multiple hostnames?  The machine can
really only have one hostname at a time (in the kernel), all other
hostnames are artificial ones that would live in /etc/hosts.  I don't
think DNS servers ever pass back more than one hostname either.  So I'd
NAK an 'as' change here because I don't think we ever need to deal with
more than one hostname at a time.

> 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

Ok.

> 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?

Might be a good thing to include some error detail or split them out
more, yes.

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

If we could use ? in function names, it would be:

NeedSecrets? (a{sa{sv}}) -> kthxbye

Maybe IdentifyRequiredSecrets() or something?

> 
> 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.

Ok.

> #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 really like GETTING; maybe just STATE_IP_CONFIG to match
NM_DEVICE_STATE_IP_CONFIG ?

Dan