[PATCH v6 08/18] tests: enhance netlink attribute test

[Dmitry V. Levin]

On Tue, Jun 27, 2017 at 06:10:10PM +0800, JingPiao Chen wrote:
[...]
> +	if (!inet_pton(AF_INET, address, msg->id.idiag_src))
> +		perror_msg_and_skip("inet_pton");
> +	if (!inet_pton(AF_INET, address, msg->id.idiag_dst))
> +		perror_msg_and_skip("inet_pton");
> +}

This can be shortened as I wrote for the previous patch.

[...]
> +static void
> +test_nla_str(const int fd)
> +{
> +	const int hdrlen = sizeof(struct inet_diag_msg);
> +	const char address[] = "12.34.56.78";
> +	struct nlmsghdr *nlh;
> +	struct nlattr *nla;
> +	unsigned int nla_len;
> +	unsigned int msg_len;
> +	void *const nlh0 = tail_alloc(NLMSG_SPACE(hdrlen));
> +	long rc;
> +
> +	nla_len = NLA_HDRLEN + 4;

I suppose this magic constant 4 ...

> +	msg_len = NLMSG_SPACE(hdrlen) + nla_len;
> +	nlh = nlh0 - nla_len;
> +	init_inet_diag_msg(nlh, msg_len, address);
> +
> +	nla = NLMSG_ATTR(nlh, hdrlen);
> +	SET_STRUCT(struct nlattr, nla,
> +		.nla_len = nla_len,
> +		.nla_type = INET_DIAG_CONG
> +	);
> +	memcpy(RTA_DATA(nla), "123", 4);

... is the same as this magic constant 4, which is the same
as sizeof("123"), ...

> +	rc = sendto(fd, nlh, msg_len, MSG_DONTWAIT, NULL, 0);
> +
> +	printf("sendto(%d, {{len=%u, type=SOCK_DIAG_BY_FAMILY"
> +	       ", flags=NLM_F_DUMP, seq=0, pid=0}, {idiag_family=AF_INET"
> +	       ", idiag_state=TCP_LISTEN, idiag_timer=0, idiag_retrans=0"
> +	       ", id={idiag_sport=htons(0), idiag_dport=htons(0)"
> +	       ", inet_pton(AF_INET, \"%s\", &idiag_src)"
> +	       ", inet_pton(AF_INET, \"%s\", &idiag_dst)"
> +	       ", idiag_if=0, idiag_cookie=[0, 0]}, idiag_expires=0"
> +	       ", idiag_rqueue=0, idiag_wqueue=0, idiag_uid=0"
> +	       ", idiag_inode=0}, {{nla_len=%u, nla_type=INET_DIAG_CONG}"
> +	       ", \"123\"}}, %u, MSG_DONTWAIT, NULL, 0) = %s\n",

... and this "123" must match the "123" used above.

In cases like this I suggest creating a static constant string and use it
instead, e.g.

	static const char str[] = "123";
	...
	nla_len = NLA_HDRLEN + sizeof(str);
	...
	memcpy(RTA_DATA(nla), str, sizeof(str));
	...
	", \"%s\"}}, %u, MSG_DONTWAIT, NULL, 0) = %s\n", ..., str, ...);

[...]
> +	/* short read of mark */
> +	nla_len = NLA_HDRLEN + sizeof(*mark);
> +	msg_len = NLMSG_SPACE(hdrlen) + nla_len;
> +	nlh = nlh0 - (nla_len - 1);
> +	init_inet_diag_msg(nlh, msg_len, address);
> +
> +	nla = NLMSG_ATTR(nlh, hdrlen);
> +	*nla = (struct nlattr) {
> +		.nla_len = nla_len,
> +		.nla_type = INET_DIAG_MARK
> +	};

This is unaligned access.

> +
> +	rc = sendto(fd, nlh, msg_len, MSG_DONTWAIT, NULL, 0);
> +
> +	printf("sendto(%d, {{len=%u, type=SOCK_DIAG_BY_FAMILY"
> +	       ", flags=NLM_F_DUMP, seq=0, pid=0}, {idiag_family=AF_INET"
> +	       ", idiag_state=TCP_LISTEN, idiag_timer=0, idiag_retrans=0"
> +	       ", id={idiag_sport=htons(0), idiag_dport=htons(0)"
> +	       ", inet_pton(AF_INET, \"%s\", &idiag_src)"
> +	       ", inet_pton(AF_INET, \"%s\", &idiag_dst)"
> +	       ", idiag_if=0, idiag_cookie=[0, 0]}, idiag_expires=0"
> +	       ", idiag_rqueue=0, idiag_wqueue=0, idiag_uid=0"
> +	       ", idiag_inode=0}, {{nla_len=%u, nla_type=INET_DIAG_MARK}"
> +	       ", %p}}, %u, MSG_DONTWAIT, NULL, 0) = %s\n",
> +	       fd, msg_len, address, address, nla_len, RTA_DATA(nla),
> +	       msg_len, sprintrc(rc));
> +
> +	/* mark */
> +	nla_len = NLA_HDRLEN + sizeof(*mark);
> +	msg_len = NLMSG_SPACE(hdrlen) + nla_len;
> +	nlh = nlh0 - nla_len;
> +	init_inet_diag_msg(nlh, msg_len, address);
> +
> +	nla = NLMSG_ATTR(nlh, hdrlen);
> +	*nla = (struct nlattr) {
> +		.nla_len = nla_len,
> +		.nla_type = INET_DIAG_MARK
> +	};

There is no unaligned access here but I suggest to use the same
initialization method for all cases like this.


-- 
ldv
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: not available
URL: <http://lists.strace.io/pipermail/strace-devel/attachments/20170628/5599aa39/attachment.bin>