On Wed, Dec 24, 2014 at 08:59:31PM +0900, Masatake YAMATO wrote:
> This patch extends -yy option; the peer address of a unix socket can be
> printed like an inet socket.
[...]
> +static bool
> +unix_parse_response(const void *data, int data_len, const unsigned long 
> inode)
> +{
> +     const struct unix_diag_msg *diag_msg = data;
> +     int rta_len = data_len - NLMSG_LENGTH(sizeof(*diag_msg));
> +     struct rtattr *attr;

I've swapped these two lines, and moved "uint32_t peer" definition after
them.  Even if compiler is capable of rearranging automatic variables for
proper alignment, code usually looks better when variables are already
aligned properly.

> +     size_t path_len = 0;
> +     char path[UNIX_PATH_MAX + 1] = { [0] = '\0' };

I'm not sure why you changed the way how "path" is initialized.
Your fist edition was OK, so I've changed it back.

> +     uint32_t peer = 0;
> +     if (diag_msg->udiag_ino != inode)
> +             return false;
> +     if (diag_msg->udiag_family != AF_UNIX)
> +             return false;
> +
> +     attr = (struct rtattr*) (diag_msg +1);
> +     while (RTA_OK(attr, rta_len)) {

BTW, this looks like a classic "for" expression.

> +             switch (attr->rta_type) {
> +             case UNIX_DIAG_NAME:
> +                     if (!path_len) {
> +                             path_len = RTA_PAYLOAD(attr);
> +                             if (path_len > UNIX_PATH_MAX)
> +                                     path_len = UNIX_PATH_MAX;
> +                             memcpy(path, RTA_DATA(attr), path_len);

There was a null-termination in your first edition, I've put it back.

> +                             break;
> +                     }

This is probably a typo, "break" statement is needed in both cases.
Fixed.

> +             case UNIX_DIAG_PEER:
> +                     if (RTA_PAYLOAD(attr) >= 4)
> +                             peer = *(uint32_t *)RTA_DATA(attr);
> +                     break;
> +             }
> +             attr = RTA_NEXT(attr, rta_len);
> +     }
> +
> +     /* prints the getting information with following format:
> +      * "UNIX:[" SELF_INODE [ "->" PEER_INODE ][ "," SOCKET_FILE ] "]" */
> +     if (peer || path_len) {
> +             tprintf("UNIX:[%lu", inode);
> +             if (peer)
> +                     tprintf("->%u", peer);
> +             if (path_len) {
> +                     if (path[0] == '\0')
> +                             tprintf(",@\"%s\"", path + 1);
> +                     else
> +                             tprintf(",\"%s\"", path);
> +             }

When I mentioned printsock(), I meant that printsock() uses string_quote():

                        if (path[0] == '\0') {
                                char *outstr = alloca(4 * path_len - 1);
                                string_quote(path + 1, outstr, -1, path_len);
                                tprintf(",@%s", outstr);
                        } else {
                                char *outstr = alloca(4 * path_len + 3);
                                string_quote(path, outstr, -1, path_len + 1);
                                tprintf(",%s", outstr);
                        }


No need to submit v5, I've applied your patch with these corrections.
Thanks!


-- 
ldv

Attachment: pgp5r6iaZYk0J.pgp
Description: PGP signature

------------------------------------------------------------------------------
Dive into the World of Parallel Programming! The Go Parallel Website,
sponsored by Intel and developed in partnership with Slashdot Media, is your
hub for all things parallel software development, from weekly thought
leadership blogs to news, videos, case studies, tutorials and more. Take a
look and join the conversation now. http://goparallel.sourceforge.net
_______________________________________________
Strace-devel mailing list
Strace-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/strace-devel

Reply via email to