Hi Ira,
> On 25 Jan 2019, at 19:02, Ira Weiny <ira.weiny@xxxxxxxxx> wrote:
>
> On Thu, Jan 24, 2019 at 03:47:35PM +0100, Håkon Bugge wrote:
>> ibacm uses ioctl with SIOCGIFCONF to enumerate the interfaces. This
>> particular ioctl will only return interfaces that are "running".
>>
>> This is a problem if, for example, ibacm is started before the
>> interfaces get plumbed, or when an interface gets resurrected.
>>
>> By using netlink (nl), we avoid using the sysfs interface to read the
>> gid, as it is supplied directly by nl.
>>
>> This commit does not change the existing functionality, but is made
>> to prepare for a fix where ibacm is unable to resurrect an interface.
>>
>> Detailed logging added:
>>
>> acm_if_iter: name: stib0 label: stib0 index: 9
>> flags: broadcast,multicast,up,running,lowerup
>> addr: 192.168.200.200/24 pkey: 0x88b4 guid: 0x21280001a17c97
>>
>> Change-Id: I6e428d1d6b2d345ccfbd74d482c69b56f8fe98da
>> Signed-off-by: Håkon Bugge <haakon.bugge@xxxxxxxxxx>
>> ---
>
> [snip]
>
>> diff --git a/ibacm/src/acm_util.c b/ibacm/src/acm_util.c
>> index f4654f12..fc22432c 100644
>> --- a/ibacm/src/acm_util.c
>> +++ b/ibacm/src/acm_util.c
>> @@ -27,48 +27,28 @@
>> * SOFTWARE.
>> */
>>
>> +#include <arpa/inet.h>
>> +#include <asm/types.h>
>> +#include <errno.h>
>> +#include <linux/netlink.h>
>> +#include <net/if.h>
>> +#include <net/if_arp.h>
>> +#include <netlink/route/addr.h>
>> +#include <netlink/route/link.h>
>> +#include <netlink/socket.h>
>> +#include <stdbool.h>
>> #include <stdio.h>
>> #include <stdlib.h>
>> -#include <net/if_arp.h>
>> #include <string.h>
>> -#include <unistd.h>
>> -#include <arpa/inet.h>
>> #include <sys/ioctl.h>
>> -#include <net/if.h>
>> #include <sys/socket.h>
>> #include <sys/types.h>
>> -#include <errno.h>
>> +#include <unistd.h>
>
> NIT:
>
> Lots of noise here in moving the includes around. It would be better if you
> did not move the include lines around so we can see that you are removing and
> adding the correct includes.
I had to insert a handful of them, and had to sort them to avoid duplicates. But I agree, nothing to do with the commit. Will fix.
On that topic, is a sort advised for includes, so dups easily can be detected? Always using <esc>X sort-lines...
> But I'm going to assume this compiles fine...
Sure does, passed our CI tests for rdma-core, which is about 100 tests with 4 different kernels...
> [snip]
>
>> +
>> +/* pre-decl to avoid compiler warning */
>> +void acm_if_iter(struct nl_object *obj, void *_ctx_and_cb);
>> +void acm_if_iter(struct nl_object *obj, void *_ctx_and_cb)
>
> What Jason said...
Sure, scope is not part of the signature. Got it!
>> +{
>> + struct ctx_and_cb *ctx_cb = (struct ctx_and_cb *)_ctx_and_cb;
>> + struct rtnl_addr *addr = (struct rtnl_addr *)obj;
>> + uint8_t bin_addr[ACM_MAX_ADDRESS] = {};
>> + char ip_str[INET6_ADDRSTRLEN];
>> + struct nl_addr *link_addr;
>> + unsigned long *guid_ptr;
>> + struct rtnl_link *link;
>> + char flags_str[128];
>> + union ibv_gid sgid;
>> + struct nl_addr *a;
>> + uint16_t pkey;
>> + int addr_len;
>> + char *label;
>> + int flags;
>> + int ret;
>> + int af;
>> +
>> + link = rtnl_link_get(link_cache, rtnl_addr_get_ifindex(addr));
>> + flags = rtnl_link_get_flags(link);
>> +
>> + if (rtnl_link_get_arptype(link) != ARPHRD_INFINIBAND)
>> + return;
>> +
>> + if (!(flags & IFF_RUNNING))
>> + return;
>> +
>> + if (!(a = rtnl_addr_get_local(addr)))
>> + return;
>> +
>> + if ((addr_len = nl_addr_get_len(a)) > ACM_MAX_ADDRESS) {
>> + acm_log(0, "address too long (%d)\n", addr_len);
>> + return;
>> + }
>> +
>> + af = nl_addr_get_family(a);
>> + if (af != AF_INET && af != AF_INET6)
>> + return;
>> +
>> + label = rtnl_addr_get_label(addr);
>> + if (!label)
>> + return;
>> +
>> + link_addr = rtnl_link_get_addr(link);
>> + memcpy(sgid.raw, nl_addr_get_binary_addr(link_addr) + 4, sizeof(sgid));
>> + guid_ptr = (unsigned long *)&sgid.raw;
>
> This does not look right to support both IPv4 and v6.
Me not understand. I am retrieving the L2 address here. It is not influenced by the choice of L3 address, is it?
>> +
>> + ret = acm_if_get_pkey(rtnl_link_get_name(link), &pkey);
>> + if (ret)
>> + return;
>> +
>> + acm_log(2, "name: %5s label: %9s index: %2d flags: %s addr: %s pkey: 0x%04x guid: 0x%lx\n",
>> + rtnl_link_get_name(link), label,
>> + rtnl_addr_get_ifindex(addr),
>> + rtnl_link_flags2str(flags, flags_str, sizeof(flags_str)),
>> + nl_addr2str(a, ip_str, sizeof(ip_str)), pkey,
>> + be64toh(*(guid_ptr + 1)));
>
> Use sgid.global.interface_id to represent the guid. That is what ibv_gid union
> is for...
Yes, thanks! Will fix.
Thanks for the review, Håkon
