Re: API changes

[Dan Williams <dcbw redhat com>]

On Thu, 2008-03-13 at 18:17 +0100, Will Stephenson wrote:
> 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.

NM pulls it right out of SIOCGIFHWADDR which is struct ether_addr (ie,
char[6]).  It does get converted from struct ether_addr to a string with
ether_ntop for D-Bus transport.

1/2 dozen to one, 6 to another.  I think the decision to be made is to
either leave it as-is (ie, strings), or convert the type to byte array
with the exact length of the device type's hardware address (char[6] for
802.11, 802.3, and 802.15, char[7] for GSM & CDMA).

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

We do this with the vpn-settings object already.  Neither NM or the
applet knows anything about the VPN connections data; only the vpn
plugin and it's properties lib do.  The options are simply passed
through.

> 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
> _______________________________________________
> NetworkManager-list mailing list
> NetworkManager-list gnome org
> http://mail.gnome.org/mailman/listinfo/networkmanager-list