[PATCHv2 2/3] Support unix sockets in -yy option

[Dmitry V. Levin]

On Wed, Dec 10, 2014 at 12:55:07PM +0900, Masatake YAMATO wrote:
> This patch extends -yy option; the peer address of a unix socket can be
> printed like an inet socket.

It works perfectly well, but there are several minor issues with the patch
I'd like to have fixed before applying.

[...]
> --- /dev/null
> +++ b/linux/unix_diag.h
> @@ -0,0 +1,25 @@
> +struct unix_diag_req {
> +	__u8	sdiag_family;
> +	__u8	sdiag_protocol;
> +	__u16	pad;
> +	__u32	udiag_states;
> +	__u32	udiag_ino;
> +	__u32	udiag_show;
> +	__u32	udiag_cookie[2];
> +};

Let's use uint8_t, uint16_t, and uint32_t instead.

[...]
> --- a/socketutils.c
> +++ b/socketutils.c
> @@ -5,9 +5,12 @@
>  #include <linux/netlink.h>
>  #include <linux/sock_diag.h>
>  #include <linux/inet_diag.h>
> +#include <linux/unix_diag.h>
> +#include <linux/rtnetlink.h>	/* rtattr */

This header file is needed not just for rtattr but also for RTA_* macros,
let's omit this comment.

> +#include <linux/un.h>		/* For UNIX_PATH_MAX */

Couldn't we include <sys/un.h> and use sizeof(struct sockaddr_un) instead?

[...]
> +		.udr = {
> +			.sdiag_family = AF_UNIX,
> +			.sdiag_protocol = 0,

All omitted fields are initialized with zero anyway, so
I suppose you can omit this .sdiag_protocol initialization.

> +			.udiag_ino = inode,
> +			.udiag_states = -1,
> +			.udiag_show = UDIAG_SHOW_NAME|UDIAG_SHOW_PEER,

Please put spaces around "|".

[...]
> +static bool
> +unix_parse_response(const void *data, int data_len, const unsigned long inode)
> +{
> +	const struct unix_diag_msg *diag_msg = data;
> +	int rtalen = data_len - NLMSG_LENGTH(sizeof(*diag_msg));

Also a style issue: "rtalen" and "data_len" do not look quite nice when
used together.  Let's rename "rtalen" to "rta_len".

> +	struct rtattr *attr;
> +	bool r, found_path;
> +	char path[UNIX_PATH_MAX + 1];
> +	uint32_t peer = 0;

This function returns "true" when it succeeds to obtain and print at least
one of attributes.  That is, if we tested these attributes instead,
"bool r" would be redundant.

> +	if (diag_msg->udiag_ino != inode)
> +		return false;
> +	if (diag_msg->udiag_family != AF_UNIX)
> +		return false;
> +	if (rtalen <= 0)
> +		return false;

RTA_OK checks the length anyway, so there is no need to do it manually.

> +	attr = (struct rtattr*) (diag_msg +1);
> +	r = false;
> +	found_path = false;
> +	while(RTA_OK(attr, rtalen))
> +	{
> +		switch (attr->rta_type)
> +		{

We put braces on line after if, while, switch, etc.

> +		case UNIX_DIAG_NAME:
> +			if (rtalen > 0)
> +			{

The length has been checked by RTA_OK already, no need to do it here.

> +				size_t l = RTA_PAYLOAD(attr);

Please avoid naming variables with a single letter "l".
A name like path_len would suit it better.
BTW, you can use this path_len instead of found_path as an indicator of
successfully obtained path.

> +				l = (l < UNIX_PATH_MAX)? l: UNIX_PATH_MAX;

Wouldn't simple "if" statement look clearer?

> +				memcpy(path, RTA_DATA(attr), l);
> +				/* convert to C string */
> +				path[l] = '\0';
> +				/* make abstract socket printable with adding prefix @*/
> +				if (path[0] == '\0')
> +					path[0] = '@';

If we just prefix path with "@" symbol, how can we distinguish an abstract
socket from a regular socket with a path name starting with this "@" symbol?
See how AF_UNIX sockets are decoded by printsock().

> +				found_path = true;
> +				r = true;
> +			}
> +			break;
> +		case UNIX_DIAG_PEER:
> +			if (RTA_PAYLOAD(attr) >= 4)
> +			{
> +				peer = *(uint32_t *)RTA_DATA(attr);
> +				r = true;
> +			}
> +			break;
> +		}
> +		attr = RTA_NEXT(attr, rtalen);
> +	}
> +
> +	/* prints the getting information with following format:
> +	   "UNIX:[" SELF_INODE [ "->" PEER_INODE ][ "," SOCKET_FILE ] "]" */

Let it be a regular boxed comment:

	/*
	 * text
	 */


-- 
ldv
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 181 bytes
Desc: not available
URL: <http://lists.strace.io/pipermail/strace-devel/attachments/20141217/3dc05a63/attachment.bin>