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 <[EMAIL PROTECTED]> > 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 > [email protected] > http://mail.gnome.org/mailman/listinfo/networkmanager-list _______________________________________________ NetworkManager-list mailing list [email protected] http://mail.gnome.org/mailman/listinfo/networkmanager-list
