Hi Luiz,
> I guess there should probably be a way to prevent systemd-udev to
> change to rename the interface
As I mentioned, I could not manage. Also makes Bluez depend on a configuration
where it is not necessary.
> or then we need kernel changes to make
> sure the interface index is sent.
It looks like the "correct" solution, but this one does not change
the kernel API.
> sleep is no acceptable in the daemon code, it should never block in
> any circumstances.
Yes, obviously.
Thank you for the review. I will prepare a new patch.
Bobby
Am Montag, 19. September 2016, 15:16:06 CEST schrieben Sie:
> Hi Bobby,
>
> On Sun, Sep 18, 2016 at 7:35 PM, Bobby Noelte <b0661n0e17e@xxxxxxxxx> wrote:
> > From: "b0661n0e71e@xxxxxxxxx" <b0661.n0e17e@xxxxxxxxx>
> >
> > Interface names may be renamed by udev (systemd-udev) after creation
> > before the interface is up. Interfaces may also be renamed when
> > the interface is down due to a bluetoothd restart.
>
> I guess there should probably be a way to prevent systemd-udev to
> change to rename the interface or then we need kernel changes to make
> sure the interface index is sent.
>
> > Use the interface index, that does not change on renaming, where possible.
> > Also do some retries when the creation of an interface fails.
> > ---
> >
> > profiles/network/bnep.c | 311
> > ++++++++++++++++++++++++++++++++++++++++-------- 1 file changed, 260
> > insertions(+), 51 deletions(-)
> >
> > diff --git a/profiles/network/bnep.c b/profiles/network/bnep.c
> > index 9bf0b18..cba0f63 100644
> > --- a/profiles/network/bnep.c
> > +++ b/profiles/network/bnep.c
> > @@ -65,6 +65,7 @@ struct bnep {
> >
> > uint16_t dst;
> > bdaddr_t dst_addr;
> > char iface[16];
> >
> > + unsigned int ifindex;
> >
> > guint attempts;
> > guint setup_to;
> > guint watch;
> >
> > @@ -98,25 +99,109 @@ int bnep_cleanup(void)
> >
> > return 0;
> >
> > }
> >
> > +/**
> > + * private: for local use only.
> > + *
> > + * bnep_indextoname() - Return interface name corresponding to interface
> > index. + * @ifindex interface index.
> > + * @iface interface name, output only, will be filled with '<unknown>' in
> > case of error. + *
> > + * The name is placed in the buffer pointed to by ifname.
> > + * The buffer must allow for the storage of at least IF_NAMESIZE bytes.
> > + *
> > + * On success 0 is returned. On error errno as a negative value is
> > returned. + */
> > +static int bnep_indextoname(unsigned int ifindex, char *iface)
> > +{
> > + int err;
> > +
> > + err = 0;
> > + if (if_indextoname(ifindex, iface) == 0) {
> > + err = -errno;
> > + strncpy(iface, "<unknown>", 10);
> > + }
> > + return err;
> > +}
> > +
> > +/**
> > + * private: for local use only.
> > + *
> > + * bnep_nametoindext() - Return interface index corresponding to
> > interface name. + * @iface interface name
> > + * @ifindex interface index, output only, will be filled with 0 in case
> > of error. + *
> > + * The interface index is placed in the variable pointed to by ifindex.
> > + *
> > + * In case the interface is temporarily unavailable do a retry.
> > + *
> > + * On success 0 is returned. On error errno as a negative value is
> > returned. + */
> > +static int bnep_nametoindex(const char *iface, unsigned int *ifindex)
> > +{
> > + int retry;
> > + int err;
> > +
> > + retry = 0;
> > + while (retry < 2) {
> > + DBG("get interface index for device %s retry %d", iface,
> > retry); + *ifindex = if_nametoindex(iface);
> > + if (*ifindex != 0) {
> > + DBG("got interface index %d for added device %s
> > retry %d", + *ifindex, iface, retry);
> > + err = 0;
> > + } else {
> > + err = -errno;
> > + }
> > + if (err != -EAGAIN)
> > + break;
> > + /* EAGAIN - Resource temporarily unavailable */
> > + retry++;
> > + sleep(1);
>
> sleep is no acceptable in the daemon code, it should never block in
> any circumstances.
>
> > + }
> > + return err;
> > +}
> > +
> >
> > static int bnep_conndel(const bdaddr_t *dst)
> > {
> >
> > + int err;
> >
> > struct bnep_conndel_req req;
> >
> > + char addr[20];
> >
> > + ba2str(dst, &addr[0]);
> >
> > memset(&req, 0, sizeof(req));
> > baswap((bdaddr_t *)&req.dst, dst);
> > req.flags = 0;
> >
> > - if (ioctl(ctl, BNEPCONNDEL, &req) < 0) {
> > - int err = -errno;
> > - error("bnep: Failed to kill connection: %s (%d)",
> > - strerror(-err),
> > -err); + err = ioctl(ctl, BNEPCONNDEL, &req);
> > + if (err < 0) {
> > + err = -errno;
> > + error("bnep: Failed to kill connection %s: %s (%d)",
> > + addr, strerror(-err), -err);
> >
> > return err;
> >
> > }
> >
> > + DBG("deleted device %s, %d", addr, err);
> > +
> >
> > return 0;
> >
> > }
> >
> > -static int bnep_connadd(int sk, uint16_t role, char *dev)
> > +/**
> > + * private: for local use only.
> > + *
> > + * bnep_connadd() - Add bluetooth ethernet connection.
> > + * @sk bluez control socket.
> > + * @role
> > + * @dev device name, template for new interface name (e.g. "bnep%d") or
> > name. + * @dst bluetooth address, only needed for retries.
> > + * @ifindex interface index, ouput only - new interface index
> > + *
> > + * In case the interface can not be accessed by the device name (e.g. due
> > to udev rename) do a retry. + *
> > + * On success 0 is returned. On error errno as a negative value is
> > returned. + */
> > +static int bnep_connadd(int sk, uint16_t role, char *dev, const bdaddr_t
> > *dst, unsigned int *ifindex)>
> > {
> >
> > struct bnep_connadd_req req;
> >
> > + int err;
> > + int retry;
> >
> > memset(&req, 0, sizeof(req));
> > strncpy(req.device, dev, 16);
> >
> > @@ -125,15 +210,40 @@ static int bnep_connadd(int sk, uint16_t role, char
> > *dev)>
> > req.sock = sk;
> > req.role = role;
> > req.flags = (1 << BNEP_SETUP_RESPONSE);
> >
> > - if (ioctl(ctl, BNEPCONNADD, &req) < 0) {
> > - int err = -errno;
> > - error("bnep: Failed to add device %s: %s(%d)",
> > - dev, strerror(-err),
> > -err);
> > - return err;
> > +
> > + retry = 0;
> > +
> > + while (retry < 2) {
> > + DBG("add device %s retry %d", req.device, retry);
> > +
> > + err = ioctl(ctl, BNEPCONNADD, &req);
> > + if (err < 0) {
> > + err = -errno;
> > + break;
> > + }
> > + DBG("added device %s retry %d", req.device, retry);
> > + /*
> > + * There is a potential race condition between udev
> > renaming the interface + * after being created by
> > BNEPCONNADD and bnep_nametoindex providing + * the
> > interface index.
> > + */
> > + err = bnep_nametoindex(&req.device[0], ifindex);
> > + if (!err)
> > + break;
> > +
> > + /* Prepare retry */
> > + retry++;
> > + bnep_conndel(dst);
> > + /* Give BT some time to really delete. */
> > + sleep(1);
>
> We aint gonna fix any race condition with sleep.
>
> > }
> >
> > - strncpy(dev, req.device, 16);
> > - return 0;
> > + if (err) {
> > + error("bnep: Failed to add device %s with interface index
> > %d: %s(%d)", + req.device, *ifindex,
> > strerror(-err), -err); + }
> > +
> > + return err;
> >
> > }
> >
> > static uint32_t bnep_getsuppfeat(void)
> >
> > @@ -148,51 +258,108 @@ static uint32_t bnep_getsuppfeat(void)
> >
> > return feat;
> >
> > }
> >
> > -static int bnep_if_up(const char *devname)
> > +/**
> > + * private: for local use only.
> > + *
> > + * bnep_if_up() - Bring interface up.
> > + * @ifindex interface index
> > + *
> > + * In case the interface can not be accessed (e.g. due to udev rename) do
> > a retry. + *
> > + * On success 0 is returned. On error errno as a negative value is
> > returned. + */
> > +static int bnep_if_up(unsigned int ifindex)
> >
> > {
> >
> > struct ifreq ifr;
> >
> > - int sk, err = 0;
> > + int sk, err;
> > + int retry;
> >
> > sk = socket(AF_INET, SOCK_DGRAM, 0);
> >
> > memset(&ifr, 0, sizeof(ifr));
> >
> > - strncpy(ifr.ifr_name, devname, IF_NAMESIZE - 1);
> > -
> >
> > ifr.ifr_flags |= IFF_UP;
> > ifr.ifr_flags |= IFF_MULTICAST;
> >
> > - if (ioctl(sk, SIOCSIFFLAGS, (void *) &ifr) < 0) {
> > + retry = 0;
> > + while (retry < 2) {
> > + err = bnep_indextoname(ifindex, ifr.ifr_name);
> > + if (err)
> > + break;
> > + /*
> > + * There is a potential race condition between udev
> > renaming the interface + * after the name is retrieved by
> > bnep_nametoindex and the usage of the + * interface name by
> > ioctl.
> > + */
> > + if (ioctl(sk, SIOCSIFFLAGS, (void *) &ifr) == 0)
> > + break;
> >
> > err = -errno;
> >
> > - error("bnep: Could not bring up %s: %s(%d)",
> > - devname, strerror(-err),
> > -err); + DBG("could not bring up interface %s with index
> > %d: %s(%d) retry %d", + ifr.ifr_name, ifindex,
> > strerror(-err), -err, retry); +
> > + /* Prepare retry */
> > + retry++;
> > + sleep(1);
>
> Ditto.
>
> > }
> >
> > close(sk);
> >
> > + if (err) {
> > + error("bnep: Could not bring up interface %s with index
> > %d: %s(%d)", + ifr.ifr_name, ifindex,
> > strerror(-err), -err); + }
> > +
> >
> > return err;
> >
> > }
> >
> > -static int bnep_if_down(const char *devname)
> > +/**
> > + * private: for local use only.
> > + *
> > + * bnep_if_down() - Bring interface down.
> > + * @ifindex interface index
> > + *
> > + * In case the interface can not be accessed (e.g. due to udev rename) do
> > a retry. + *
> > + * On success 0 is returned. On error errno as a negative value is
> > returned. + */
> > +static int bnep_if_down(unsigned int ifindex)
> >
> > {
> >
> > struct ifreq ifr;
> >
> > - int sk, err = 0;
> > + int sk, err;
> > + int retry;
> >
> > sk = socket(AF_INET, SOCK_DGRAM, 0);
> >
> > memset(&ifr, 0, sizeof(ifr));
> >
> > - strncpy(ifr.ifr_name, devname, IF_NAMESIZE - 1);
> > -
> >
> > ifr.ifr_flags &= ~IFF_UP;
> >
> > - /* Bring down the interface */
> > - if (ioctl(sk, SIOCSIFFLAGS, (void *) &ifr) < 0) {
> > + retry = 0;
> > + while (retry < 2) {
> > + err = bnep_indextoname(ifindex, ifr.ifr_name);
> > + if (err)
> > + break;
> > + /*
> > + * There is a potential race condition between udev
> > renaming the interface + * after the name is retrieved by
> > bnep_nametoindex and the usage of the + * interface name by
> > ioctl.
> > + */
> > + if (ioctl(sk, SIOCSIFFLAGS, (void *) &ifr) == 0)
> > + break;
> >
> > err = -errno;
> >
> > - error("bnep: Could not bring down %s: %s(%d)",
> > - devname, strerror(-err),
> > -err); + DBG("could not bring down interface %s with index
> > %d: %s(%d) retry %d", + ifr.ifr_name, ifindex,
> > strerror(-err), -err, retry); +
> > + /* Prepare retry */
> > + retry++;
> > + sleep(1);
>
> Ditto.
>
> > }
> >
> > close(sk);
> >
> > + if (err) {
> > + error("bnep: Could not bring down interface %s with index
> > %d: %s(%d)", + ifr.ifr_name, ifindex,
> > strerror(-err), -err); + }
> > +
> >
> > return err;
> >
> > }
> >
> > @@ -270,14 +437,19 @@ static gboolean bnep_setup_cb(GIOChannel *chan,
> > GIOCondition cond,>
> > setsockopt(sk, SOL_SOCKET, SO_RCVTIMEO, &timeo, sizeof(timeo));
> >
> > sk = g_io_channel_unix_get_fd(session->io);
> >
> > - if (bnep_connadd(sk, session->src, session->iface) < 0)
> > + if (bnep_connadd(sk, session->src, session->iface,
> > &session->dst_addr, &session->ifindex) < 0)>
> > goto failed;
> >
> > - if (bnep_if_up(session->iface) < 0) {
> > + if (bnep_if_up(session->ifindex) < 0) {
> >
> > bnep_conndel(&session->dst_addr);
> > goto failed;
> >
> > }
> >
> > + /* Update interface name to current one only after interface is
> > up.
> > + * Only at that time the name should be stable (no udev rename).
> > + */
> > + bnep_indextoname(session->ifindex, session->iface);
> > +
> >
> > session->watch = g_io_add_watch(session->io,
> >
> > G_IO_ERR | G_IO_HUP | G_IO_NVAL,
> > (GIOFunc) bnep_watchdog_cb,
> > session);
> >
> > @@ -305,7 +477,7 @@ static int bnep_setup_conn_req(struct bnep *session)
> >
> > req = (void *) pkt;
> > req->type = BNEP_CONTROL;
> > req->ctrl = BNEP_SETUP_CONN_REQ;
> >
> > - req->uuid_size = 2; /* 16bit UUID */
> > + req->uuid_size = 2; /* 16bit UUID */
> >
> > s = (void *) req->service;
> > s->src = htons(session->src);
> > s->dst = htons(session->dst);
> >
> > @@ -354,6 +526,7 @@ struct bnep *bnep_new(int sk, uint16_t local_role,
> > uint16_t remote_role,>
> > session->dst = remote_role;
> > strncpy(session->iface, iface, 16);
> > session->iface[15] = '\0';
> >
> > + session->ifindex = 0;
> >
> > g_io_channel_set_close_on_unref(session->io, TRUE);
> > session->watch = g_io_add_watch(session->io,
> >
> > @@ -430,20 +603,26 @@ void bnep_disconnect(struct bnep *session)
> >
> > session->io = NULL;
> >
> > }
> >
> > - bnep_if_down(session->iface);
> > + bnep_if_down(session->ifindex);
> >
> > bnep_conndel(&session->dst_addr);
> >
> > }
> >
> > -static int bnep_add_to_bridge(const char *devname, const char *bridge)
> > +static int bnep_add_to_bridge(unsigned int ifindex, const char *bridge)
> >
> > {
> >
> > - int ifindex;
> >
> > struct ifreq ifr;
> > int sk, err = 0;
> >
> > + char devname[IF_NAMESIZE];
> >
> > - if (!devname || !bridge)
> > + DBG("add interface index %d to bridge %s", ifindex, bridge);
> > + if (!ifindex || !bridge)
> >
> > return -EINVAL;
> >
> > - ifindex = if_nametoindex(devname);
> > + err = bnep_indextoname(ifindex, &devname[0]);
> > + if (err) {
> > + error("bnep: Can't add interface %s with index %d to the
> > bridge %s: %s(%d)", + devname,
> > ifindex, bridge, strerror(-err), -err); + return -EINVAL;
> > + }
> >
> > sk = socket(AF_INET, SOCK_STREAM, 0);
> > if (sk < 0)
> >
> > @@ -451,7 +630,8 @@ static int bnep_add_to_bridge(const char *devname,
> > const char *bridge)>
> > memset(&ifr, 0, sizeof(ifr));
> > strncpy(ifr.ifr_name, bridge, IFNAMSIZ - 1);
> >
> > - ifr.ifr_ifindex = ifindex;
> > + ifr.ifr_ifindex = (int)ifindex;
> > +
> >
> > if (ioctl(sk, SIOCBRADDIF, &ifr) < 0) {
> >
> > err = -errno;
> >
> > @@ -466,16 +646,21 @@ static int bnep_add_to_bridge(const char *devname,
> > const char *bridge)>
> > return err;
> >
> > }
> >
> > -static int bnep_del_from_bridge(const char *devname, const char *bridge)
> > +static int bnep_del_from_bridge(unsigned int ifindex, const char *bridge)
> >
> > {
> >
> > - int ifindex;
> >
> > struct ifreq ifr;
> > int sk, err = 0;
> >
> > + char devname[IF_NAMESIZE];
> >
> > - if (!devname || !bridge)
> > + if (!ifindex || !bridge)
> >
> > return -EINVAL;
> >
> > - ifindex = if_nametoindex(devname);
> > + err = bnep_indextoname(ifindex, &devname[0]);
> > + if (err) {
> > + error("bnep: Can't delete interface %s with index %d from
> > the bridge %s: %s(%d)", + devname,
> > ifindex, bridge, strerror(-err), -err); + return -EINVAL;
> > + }
> >
> > sk = socket(AF_INET, SOCK_STREAM, 0);
> > if (sk < 0)
> >
> > @@ -483,7 +668,7 @@ static int bnep_del_from_bridge(const char *devname,
> > const char *bridge)>
> > memset(&ifr, 0, sizeof(ifr));
> > strncpy(ifr.ifr_name, bridge, IFNAMSIZ - 1);
> >
> > - ifr.ifr_ifindex = ifindex;
> > + ifr.ifr_ifindex = (int)ifindex;
> >
> > if (ioctl(sk, SIOCBRDELIF, &ifr) < 0) {
> >
> > err = -errno;
> >
> > @@ -606,6 +791,7 @@ static int bnep_server_add_legacy(int sk, uint16_t
> > dst, char *bridge,>
> > char *iface, const bdaddr_t *addr,
> > uint8_t *setup_data, int len)
> >
> > {
> >
> > + unsigned int ifindex;
> >
> > int err, n;
> > uint16_t rsp;
> >
> > @@ -616,27 +802,32 @@ static int bnep_server_add_legacy(int sk, uint16_t
> > dst, char *bridge,>
> > goto reply;
> >
> > }
> >
> > - err = bnep_connadd(sk, dst, iface);
> > + err = bnep_connadd(sk, dst, iface, addr, &ifindex);
> >
> > if (err < 0) {
> >
> > rsp = BNEP_CONN_NOT_ALLOWED;
> > goto reply;
> >
> > }
> >
> > - err = bnep_add_to_bridge(iface, bridge);
> > + err = bnep_add_to_bridge(ifindex, bridge);
> >
> > if (err < 0) {
> >
> > bnep_conndel(addr);
> > rsp = BNEP_CONN_NOT_ALLOWED;
> > goto reply;
> >
> > }
> >
> > - err = bnep_if_up(iface);
> > + err = bnep_if_up(ifindex);
> >
> > if (err < 0) {
> >
> > - bnep_del_from_bridge(iface, bridge);
> > + bnep_del_from_bridge(ifindex, bridge);
> >
> > bnep_conndel(addr);
> > rsp = BNEP_CONN_NOT_ALLOWED;
> > goto reply;
> >
> > }
> >
> > + /* Update interface name to current one only after interface is
> > up.
> > + * Only at that time the name should be stable (no udev rename).
> > + */
> > + bnep_indextoname(ifindex, iface);
> > +
> >
> > rsp = BNEP_SUCCESS;
> >
> > reply:
> > @@ -652,6 +843,7 @@ reply:
> > int bnep_server_add(int sk, char *bridge, char *iface, const bdaddr_t
> > *addr,>
> > uint8_t *setup_data, int
> > len)
> >
> > {
> >
> > + unsigned int ifindex;
> >
> > int err;
> > uint32_t feat;
> > uint16_t rsp, dst;
> >
> > @@ -691,24 +883,29 @@ int bnep_server_add(int sk, char *bridge, char
> > *iface, const bdaddr_t *addr,>
> > return bnep_server_add_legacy(sk, dst, bridge, iface,
> > addr,
> >
> > setup_data, len);
> >
> > - err = bnep_connadd(sk, dst, iface);
> > + err = bnep_connadd(sk, dst, iface, addr, &ifindex);
> >
> > if (err < 0) {
> >
> > rsp = BNEP_CONN_NOT_ALLOWED;
> > goto failed;
> >
> > }
> >
> > - err = bnep_add_to_bridge(iface, bridge);
> > + err = bnep_add_to_bridge(ifindex, bridge);
> >
> > if (err < 0)
> >
> > goto failed_conn;
> >
> > - err = bnep_if_up(iface);
> > + err = bnep_if_up(ifindex);
> >
> > if (err < 0)
> >
> > goto failed_bridge;
> >
> > + /* Update interface name to current one only after interface is
> > up.
> > + * Only at that time the name should be stable (no udev rename).
> > + */
> > + bnep_indextoname(ifindex, iface);
> > +
> >
> > return 0;
> >
> > failed_bridge:
> > - bnep_del_from_bridge(iface, bridge);
> > + bnep_del_from_bridge(ifindex, bridge);
> >
> > failed_conn:
> > bnep_conndel(addr);
> >
> > @@ -727,10 +924,22 @@ failed:
> > void bnep_server_delete(char *bridge, char *iface, const bdaddr_t *addr)
> > {
> >
> > + int err;
> > + unsigned int ifindex;
> > +
> >
> > if (!bridge || !iface || !addr)
> >
> > return;
> >
> > - bnep_del_from_bridge(iface, bridge);
> > - bnep_if_down(iface);
> > + err = bnep_nametoindex(iface, &ifindex);
> > + if (err) {
> > + error("bnep: Failed to delete device %s - interface not
> > available/ renamed: %s(%d)", +
> > iface, strerror(-err), -err); + goto failed;
> > + }
> > +
> > + bnep_del_from_bridge(ifindex, bridge);
> > + bnep_if_down(ifindex);
> >
> > +failed:
> > bnep_conndel(addr);
> >
> > }
> >
> > +
> > --
> > 2.7.4
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-bluetooth"
> > in the body of a message to majordomo@xxxxxxxxxxxxxxx
> > More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
