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
pgpNHDwvPyX40.pgp
Description: PGP signature
------------------------------------------------------------------------------ Download BIRT iHub F-Type - The Free Enterprise-Grade BIRT Server from Actuate! Instantly Supercharge Your Business Reports and Dashboards with Interactivity, Sharing, Native Excel Exports, App Integration & more Get technology previously reserved for billion-dollar corporations, FREE http://pubads.g.doubleclick.net/gampad/clk?id=164703151&iu=/4140/ostg.clktrk
_______________________________________________ Strace-devel mailing list Strace-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/strace-devel
