Re: [Linuxptp-devel] [PATCH 2/8] rtnl: extend struct interface and add new function rtnl_link_info

[Richard C. <ric...@gm...>]

This patch has a number of coding style issues...

On Sat, Jun 24, 2017 at 04:48:30PM +0800, Hangbin Liu wrote:
> diff --git a/rtnl.c b/rtnl.c
> index 39faeb7..971f273 100644
> --- a/rtnl.c
> +++ b/rtnl.c
> @@ -18,8 +18,6 @@
>   */
>  #include <asm/types.h>
>  #include <sys/socket.h> /* Must come before linux/netlink.h on some systems. */
> -#include <linux/netlink.h>
> -#include <linux/rtnetlink.h>
>  #include <net/if.h>
>  #include <stdio.h>
>  #include <stdlib.h>
> @@ -84,6 +82,60 @@ int rtnl_link_query(int fd, unsigned int if_index)
>  	return 0;
>  }
>  
> +static inline __u32 rta_getattr_u32(const struct rtattr *rta)
> +{
> +	return *(__u32 *)RTA_DATA(rta);
> +}

A function must be followed by one blank line.

> +static inline const char *rta_getattr_str(const struct rtattr *rta)
> +{
> +	return (const char *)RTA_DATA(rta);
> +}

Here too.

> +int rtnl_rtattr_parse(struct rtattr *tb[], int max, struct rtattr *rta, int len)
> +{
> +	unsigned short type;
> +
> +	memset(tb, 0, sizeof(struct rtattr *) * (max + 1));
> +	while (RTA_OK(rta, len)) {
> +		type = rta->rta_type;
> +		if ((type <= max) && (!tb[type]))
> +			tb[type] = rta;
> +		rta = RTA_NEXT(rta, len);
> +	}
> +	if (len)
> +		fprintf(stderr, "!!!Deficit %d, rta_len=%d\n",
> +			len, rta->rta_len);
> +	return 0;
> +}
> +
> +int rtnl_linkinfo_parse(struct rtattr *rta, char *device)
> +{
> +	int index;
> +	struct rtattr *linkinfo[IFLA_INFO_MAX+1];
> +	struct rtattr *bond[IFLA_BOND_MAX+1];

array[MAX + 1];
Spaces   ^^^

> +	rtnl_nested_rtattr_parse(linkinfo, IFLA_INFO_MAX, rta);
> +
> +	if (linkinfo[IFLA_INFO_KIND]) {
> +		const char *kind = rta_getattr_str(linkinfo[IFLA_INFO_KIND]);
> +
> +		if (kind && !strncmp(kind, "bond", 4) &&
> +		    linkinfo[IFLA_INFO_DATA]) {
> +			rtnl_nested_rtattr_parse(bond, IFLA_BOND_MAX,
> +						 linkinfo[IFLA_INFO_DATA]);
> +
> +			if (bond[IFLA_BOND_ACTIVE_SLAVE]) {
> +				index = rta_getattr_u32(bond[IFLA_BOND_ACTIVE_SLAVE]);
> +
> +				if (!if_indextoname(index, device)) {
> +					pr_err("failed to get device name: %m");
> +					return -1;
> +				}
> +			}
> +		}
> +	}
> +	return 0;
> +}
> +
>  int rtnl_link_status(int fd, rtnl_callback cb, void *ctx)
>  {
>  	int index, len;
> @@ -92,6 +144,18 @@ int rtnl_link_status(int fd, rtnl_callback cb, void *ctx)
>  	struct msghdr msg;
>  	struct nlmsghdr *nh;
>  	struct ifinfomsg *info = NULL;
> +	char *device;
> +	struct rtattr *tb[IFLA_MAX+1];
> +
> +	if (cb)
> +		device = calloc(1, sizeof(MAX_IFNAME_SIZE + 1));
> +	else
> +		device = (char *)ctx;
> +
> +	if(!device) {

One space after keywords please.

> +		fprintf(stderr, "rtnl: no enought memory for device name\n");
> +		return -1;
> +	}
>  
>  	if (!rtnl_buf) {
>  		rtnl_len = 4096;

> @@ -38,6 +43,26 @@ int rtnl_close(int fd);
>  int rtnl_link_query(int fd, unsigned int index);
>  
>  /**
> + * Parase the rtattr info
> + * @param tb     rtattr array.
> + * @param max    max type of ratttr.
> + * @param rta    rta header
> + * @param len    rta playload length
> + * @return       Zero on success, or -1 on error.
> + */
> +int rtnl_rtattr_parse(struct rtattr *tb[], int max, struct rtattr *rta, int len);

This function should be private in rtnl.c.

> +#define rtnl_nested_rtattr_parse(tb, max, rta) \
> +	(rtnl_rtattr_parse((tb), (max), RTA_DATA(rta), RTA_PAYLOAD(rta)))

This also.

> +/**
> + * Parase the link info
> + * @param rta    rta header
> + * @param device interface name.
> + * @return       Zero on success, or -1 on error.
> + */
> +int rtnl_linkinfo_parse(struct rtattr *rta, char *device);

This also.

Thanks,
Richard