Re: API changes

[Will Stephenson <wstephenson kde org>]

On Thursday 13 March 2008, Tambet Ingo said:
> 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).

I'd keep the clean internal structure and offer a OO activate method by 
offering the dbus interface for Device via a facade object that has the 
back-reference to NMManager, but you're the one writing the code here.

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

Agreed

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

Ok, I'll explain this in the API docu.

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

I understood the applet merges APs in the same network for presentation to the 
user, is that what all the calls to ether_aton() in the NM and applet sources 
are doing?  And HAL uses uint64 for mac addresses. At a guess, you're 
converting from that, using hex-and-colons for dbus transport, converting it 
to ether_addr for comparisons, and converting it back again for the UI.

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

Of course, doing it in one class would be impossible.  But strings don't do 
anything differently to uints in for extensibility - you still have to 
prevent collisions.  Do you want to allow plugins/extensions to define their 
own groups, or allow them to add new settings within existing settings 
groups?  That might cause problems.  We can assume any one plugin isn't going 
to wantonly reuse an existing key from core NM, so the collision problem is 
going to be two competing plugins extending the same group.  Keeping the set 
of keys in a group fixed would be safe.

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

Sorry, was thinking of IPv4config in a general sense - if you had the results 
of a dns lookup in here you could very well have more than one name.  But in 
the NM DHCP context it's not relevant.

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

Ok.

Will