On 07/06/16(Tue) 11:53, Gerhard Roth wrote:
> On Mon, 6 Jun 2016 10:30:11 +0200 Martin Pieuchot <[email protected]> wrote:
> > On 01/06/16(Wed) 17:20, Gerhard Roth wrote:
> > Any reason for using a different license in documentation an code?
>
> Different in what sense? Which paragraph do you mean?
This is a BSD 2-clauses the code is ISC.
> I just copied it from some other man page in share/man/man4. Can't
> remember which one it was. But they all say pretty much the same thing.
Well if you don't mind, please use /usr/share/misc/license.template :)
> > > +/*
> > > + * Some devices are picky about too frequent control messages.
> > > + * Query device state not more often than every 0.5 secs.
> > > + */
> > > +struct timeval umbim_update_rate = { 0, 500000 };
> > ^^^^^^^^^^^^^^^^^
> > This variable seems unused. What it is for?
>
> Some remnant from an earlier version.
>
>
> >
> > > +int umbim_delay = 4000;
> >
> > What is this supposed to be?
>
> A value?
>
> A litte delay in between the requests sent to the device.
> Using a global variable helps to tune the value via DDB in the
> development stage.
You're paraphrasing your code :) My unclear question was just why
do you need such delay? ;)
> > > + break;
> > > + case SIOCSIFFLAGS:
> > > + usb_add_task(sc->sc_udev, &sc->sc_umbim_task);
> >
> > I guess you want to wait for the task to complete here. You're not
> > respecting the API by returning directly. But why using a task? To
> > serialize up/down operations?
>
> No, waiting here is not an option. In order to get a complete configuration
> of the device you need a mobile connection with the next base station.
> Would you really want "ifconfig up" to block indefinitely when you're in
> the basement without network access?
>
> The task is there, because going "up" needs a lot of steps with
> requests being sent to device and then waiting for the device to
> respond to them.
I understand you point. My concern is just that SIOCSIFFLAGS indicates
that the interface is down/up once the ioclt(2) is finished. It doesn't
seem to be the case in your driver, I just hope this is not a problem.
> > > + if (ifr->ifr_mtu > ifp->if_hardmtu) {
> > > + error = EINVAL;
> > > + break;
> > > + }
> > > + ifp->if_mtu = ifr->ifr_mtu;
> > > + break;
> > > + case SIOCGIFMTU:
> > > + ifr->ifr_mtu = ifp->if_mtu;
> > > + break;
> > > + case SIOCGIFHARDMTU:
> > > + ifr->ifr_hardmtu = ifp->if_hardmtu;
> > > + break;
> > > + case SIOCAIFADDR:
> >
> > It is accepted that this ioctl(2) implicitly bring the interface up.
>
> But this is a point-to-point interface and behaves a little bit
> different from regular Ethernet interfaces. You're not supposed to
> add IP destination addresses manually. Your provider will tell them to you.
> And in case you do try to add them manually, that will not get a
> working network interface.
I'm not saying it makes sense. I'm just explaining how it is.
> > What about routing domains? Did you try your device in a rdomain != 0?
>
> No I didn't try them. But what could happen? This is a point-to-point
> interface. No more routing decisions should be necessary on that level.
What could happen? Good question. I don't know.
> > > +void
> > > +umbim_linkstate(struct umbim_softc *sc, int state)
> > > +{
> > > + struct ifnet *ifp = GET_IFP(sc);
> > > + int s;
> > > +
> > > + s = splnet();
> > > + if (ifp->if_link_state != state) {
> > > + log(LOG_INFO, "%s: link state changed from %s to %s\n",
> > > + DEVNAM(sc),
> > > + LINK_STATE_IS_UP(ifp->if_link_state) ? "up" : "down",
> > > + LINK_STATE_IS_UP(state) ? "up" : "down");
> > > + ifp->if_link_state = state;
> > > + if_link_state_change(ifp);
> > > + if (!LINK_STATE_IS_UP(state)) {
> > > + memset(sc->sc_info.ipv4dns, 0,
> > > + sizeof (sc->sc_info.ipv4dns));
> > > + umbim_restore_ipv4addr(sc);
> >
> > Why do you need to change the address based on the link state?
>
> Well, once you're connected, your provider will give you an IP address
> and the driver will put that address onto the network interface.
>
> But when you lose the connection with the network or you manually set
> the interface down, this IP address once given to you by the provider
> is no longer yours and must not be used anymore.
So why restore? Don't you want to forget the current address? I don't
get it.
> > Anyway you current code is racy because if_link_state_change() is
> > asynchronous.
>
> Right. It's better to update the IP address first and then call
> if_link_state_change() afterwards.
In your state machine you says that your link state is UP when you have
an IP address assigned, right? I'm confused about that.
> > > +void
> > > +umbim_set_ipv4addr(struct ifnet *ifp, struct umbim_ifaddr *uif, char *op)
> > > +{
> > > + struct ifreq ifr;
> > > + int error;
> > > +
> > > + splassert(IPL_NET);
> > > +
> > > + if (uif == NULL)
> > > + return;
> > > + log(LOG_INFO, "%s: %s IPv4 addr %s, mask %s, gateway %s\n",
> > > + DEVNAM(ifp->if_softc), op,
> > > + umbim_ntop((struct sockaddr *)&uif->addr),
> > > + umbim_ntop((struct sockaddr *)&uif->netmask),
> > > + umbim_ntop((struct sockaddr *)&uif->dst));
> > > +
> > > + memset(&ifr, 0, sizeof (ifr));
> > > + memcpy(&ifr.ifr_addr, &uif->addr, sizeof (ifr.ifr_addr));
> > > + if ((error = in_ioctl(SIOCSIFADDR, (caddr_t)&ifr, ifp, 1)) != 0)
> > > + log(LOG_ERR, "%s: unable to set IPv4 address, error %d\n",
> > > + DEVNAM(ifp->if_softc), error);
> >
> > These ioctl(2) are deprecated and should not be used. That's
> > why I motioned SIOCAIFADDR in my previous email.
>
> But SIOCAIFADDR is not the same. If will *add* another address instead
> of changing the current on.
It doesn't matter, that's what should be used.
> Suppose I had this configuration:
>
> inet 0.0.0.1 --> 0.0.0.2 netmask 0xff000000
Having this doesn't make sense.
> Now I get an IP address from my provider, I want something like this:
>
> inet 10.75.178.41 --> 10.75.178.42 netmask 0xfffffffc
>
> But if I used SIOCAIFADDR I would get this instead:
>
> inet 0.0.0.1 --> 0.0.0.2 netmask 0xff000000
> inet 10.75.178.41 --> 10.75.178.42 netmask 0xfffffffc
>
> And deleting the old one first seems more racy.
Why?
> > > + memset(&ifr, 0, sizeof (ifr));
> > > + memcpy(&ifr.ifr_dstaddr, &uif->dst, sizeof (ifr.ifr_dstaddr));
> > > + if ((error = in_ioctl(SIOCSIFDSTADDR, (caddr_t)&ifr, ifp, 1)) != 0)
> > > + log(LOG_ERR, "%s: unable to set IPv4 gateway, error %d\n",
> > > + DEVNAM(ifp->if_softc), error);
> > > +
> > > + memset(&ifr, 0, sizeof (ifr));
> > > + memcpy(&ifr.ifr_addr, &uif->netmask, sizeof (ifr.ifr_addr));
> > > + if ((error = in_ioctl(SIOCSIFNETMASK, (caddr_t)&ifr, ifp, 1)) != 0)
> > > + log(LOG_ERR, "%s: unable to set IPv4 netmask, error %d\n",
> > > + DEVNAM(ifp->if_softc), error);
> >
> > Please return an error instead of using log(9) for error reporting.
>
> We're somewhere in a callback handler for parsing an processing
> unsolicited notifications from the device. There is no one there,
> whom I could return the error to.
Too bad because we *do not* log(9) every driver failure.
> > > + if (newifaddr || ifaddr_changed) {
> > > + memset(&uif, 0, sizeof (uif));
> > > + uif.addr.sin_family = AF_INET;
> > > + uif.addr.sin_len = sizeof(uif.addr);
> > > + uif.addr.sin_addr.s_addr = ipv4elem.addr;
> > > + uif.dst.sin_family = AF_INET;
> > > + uif.dst.sin_len = sizeof(uif.dst);
> > > + if (avail & UMBIM_IPCONF_HAS_GWINFO) {
> > > + off = letoh32(ic->ipv4_gwoffs);
> > > + uif.dst.sin_addr.s_addr =
> > > + letoh32(*((uint32_t *)(data + off)));
> > > + } else if (newifaddr)
> > > + uif.dst.sin_addr.s_addr = INADDR_BROADCAST;
> > > + uif.netmask.sin_family = AF_INET;
> > > + uif.netmask.sin_len = sizeof(uif.netmask);
> > > + in_len2mask(&uif.netmask.sin_addr, ipv4elem.prefixlen);
> > > + umbim_set_ipv4addr(ifp, &uif, newifaddr ? "new" : "change");
> >
> > I still fail to understand why you keep track of the previous address.
>
> Just a said above, we start with some fake address:
>
> inet 0.0.0.1 --> 0.0.0.2 netmask 0xff000000
Well don't to that.
> Once registered with the network provider, we get an IP address:
>
> inet 10.75.178.41 --> 10.75.178.42 netmask 0xfffffffc
>
> And now we de-register from the network. The IP address given to us
> by the provider cannot be used anymore. So we want to restore the
> original one (0.0.0.1 --> 0.0.0.2).
Don't restore anything.
> Maybe we don't need the initial fake IP address. But this wasn't my
> invention; it is just the way other point-to-point interfaces do it
I know and I'm sorry for you but I'm fighting against this pattern. So
I'm not in favor of yet-another-copy of it.
> > Could you move that to SIOCSIFMTU, this would already be ready when a
> > userland tool will parse the messages.
>
> No that's not possible. That is not the way that MBIM is working.
>
> We get into this function once the device negotioated an IP configuration
> with the provider network and feels ready to inform us with an
> unsolicited MBIM_CID_IP_CONFIGURATION message.
>
> There is no way to force the device to generate this message on our
> request.
>
> So when we receive it, we might have to adjust the MTU of the
> interface or else we might not be able to send further data traffic.
You're assuming that your driver will stay like it is. I though that
you wanted to offload the configuration logic to userland. That's why
I'm asking you to use the existing ioctl(2) interface to ease the future
transition.
All I'm saying is that you could call ifp->if_ioctl(SIOCSIFMTU).
Like you could split the "driver" part from the p2p logic part in your
code. Of course this will introduce a "space naming violation" but it
encourage people to re-use your code.
> > > +/*
> > > + * Diagnostic routines
> > > + */
> > > +char *
> > > +umbim_ntop(struct sockaddr *sa)
> > > +{
> > > +#define NUMBUFS 4
> > > + static char astr[NUMBUFS][INET_ADDRSTRLEN];
> > > + static unsigned nbuf = 0;
> > > + char *s;
> > > +
> > > + s = astr[nbuf++];
> > > + if (nbuf >= NUMBUFS)
> > > + nbuf = 0;
> > > +
> > > + switch (sa->sa_family) {
> > > + case AF_INET:
> > > + default:
> > > + inet_ntop(AF_INET, &satosin(sa)->sin_addr, s, sizeof (astr[0]));
> > > + break;
> > > + case AF_INET6:
> > > + inet_ntop(AF_INET6, &satosin6(sa)->sin6_addr, s,
> > > + sizeof (astr[0]));
> > > + break;
> > > + }
> > > + return s;
> > > +}
> >
> > Please do not introduce this function. The generic one has been removed
> > in order to stop using static buffers, introducing a new one will on
> > lead to confusion.
>
> I can't see any danger here as I know exactly how many static buffers are
> required.
You're clever. People reading your code are not and do copy/paste.
That's the danger, a bad advice.
