[haiku-commits] Re: r40392 - haiku/trunk/src/tests/kits/net/preflet/InterfacesAddOn
- From: Alexander von Gluck <kallisti5@xxxxxxxxxxx>
- To: haiku-commits@xxxxxxxxxxxxx
- Date: Tue, 08 Feb 2011 14:21:03 -0600
On Tue, 2011-02-08 at 20:31 +0100, Axel Dörfler wrote:
> kallisti5@xxxxxxxxxxx wrote:
> > @@ -107,6 +97,9 @@
> > BBitmap* stateIcon(NULL);
> > BString interfaceState;
> >
> > + BNetworkAddress addrIPv4 = fSettings->GetAddr(AF_INET);
> > + BNetworkAddress addrIPv6 = fSettings->GetAddr(AF_INET6);
> > +
> > if (!list)
> > return;
>
> We don't put anything but a single space between the type and the
> variable.
Sorry, old habit of aligning variable names, fixed.
> Besides this, this code looks odd; variables should be
> declared where they are needed, not at the top of functions.
> Especially when the condition could easily be checked before.
You're right that these could move down in the sources in this case,
fixed.
> > - ifreq request;
> > - if (!_PrepareRequest(request))
> > + ifreq ifReq;
> > + sockaddr_in* inetAddress = NULL;
> > +
> > + if (!_PrepareRequest(ifReq))
> > return;
>
> Same issues here, besides that I like 'request' better than ifReq.
changed.
> > + fIPv4Addr = *((sockaddr_in *)(&(ifReq.ifr_addr)));
>
> Way too many parenthesis here - a single pair is all you need in this
> line.
> You do this again later in the code.
fixed.
> BTW I can recommend switching to BNetworkInterface instead - this makes
> all that code a whole lot more readable, and is also a lot easier to
> use and maintain.
+1, I'll see if I can change it.
>
> > - ifconf config;
> > - config.ifc_len = sizeof(config.ifc_value);
> > - if (ioctl(fSocket, SIOCGRTSIZE, &config, sizeof(struct ifconf)) <
> > 0)
> > + ifconf ifCfg;
>
> Same here, "ifCfg" is not a nice name; either resemble the structure
> name as you did with ifReq, or use real words like
> interfaceConfiguration if you want to be verbose.
> IMO 'config' is a much better compromise, though.
>
> > +BNetworkAddress
> > +NetworkSettings::GetAddr(int family)
>
> I haven't looked closely, but it looks like that class could just return
> BNetworkAddresses directly instead.
It returns a different BNetworkAddress based on AF_INET or AF_INET6... I
thought that conditional statement was too much for a header file
function.
(this might change when moving to BNetworkInterface though.)
> > +const char*
> > +NetworkSettings::GetIP(int family)
> [...]
> > +const char*
> > +NetworkSettings::GetNetmask(int family)
> [...]
> > +int32
> > +NetworkSettings::GetPrefixLen(int family)
>
> Besides that, you changed the correct naming scheme (without the Get*)
> to one that doesn't match our other classes.
> Get*() should only be used if the operation can fail, and you don't
> directly return the object (but use a reference parameter instead).
eeh. I thought GetIP, SetIP, etc were more consistent... I'll change
that though.
> Bye,
> Axel.
Thanks! I took a break because I figured something was off with that
many changes.
My end goal is to have NetworkSettings handle reading the current
network state and setting a new network state. (which I think was it's
direction originally) This will also make handling profiles easier as
the settings are all stored in one place.
-- Alex
Other related posts:
