On Mon, Jun 13, 2016 at 02:37:21PM +0000, Fabien Siron wrote:
> This commit aims to avoid future string comparisons with getfdproto function
> in changing its return type. It also adapts the functions that use it.
>
> * defs.h (print_sockaddr_by_inode): Add.
> * socketutils.h (socket_protocols): New enum.
> * socketutils.c (print_sockaddr_by_inode): Move proto to int.
> * util.c (socket_protocol_strings): New global structure.
> (getfdproto): Return int instead of string.
> (printfd): Change type of proto.
> ---
> defs.h | 2 +-
> socketutils.c | 61
> ++++++++++++++++++++++++++++++++++++++---------------------
> socketutils.h | 42 ++++++++++++++++++++++++++++++++++++++++
> util.c | 34 ++++++++++++++++++++++++---------
> 4 files changed, 107 insertions(+), 32 deletions(-)
> create mode 100644 socketutils.h
>
> diff --git a/defs.h b/defs.h
> index f7a85ca..594a292 100644
> --- a/defs.h
> +++ b/defs.h
> @@ -631,7 +631,7 @@ extern void printpathn(struct tcb *, long, unsigned int);
> #define TIMESPEC_TEXT_BUFSIZE \
> (sizeof(intmax_t)*3 * 2 + sizeof("{tv_sec=%jd, tv_nsec=%jd}"))
> extern void printfd(struct tcb *, int);
> -extern bool print_sockaddr_by_inode(const unsigned long, const char *);
> +extern bool print_sockaddr_by_inode(const unsigned long, const int);
As this new enum is going to be exposed this way to every user via defs.h,
let's define this enum in defs.h, too.
> extern bool print_sockaddr_by_inode_cached(const unsigned long);
> extern void print_dirfd(struct tcb *, int);
> extern void printsock(struct tcb *, long, int);
> diff --git a/socketutils.c b/socketutils.c
> index 5d8d3ed..0ac8463 100644
> --- a/socketutils.c
> +++ b/socketutils.c
> @@ -438,44 +438,61 @@ netlink_print(const int fd, const unsigned long inode)
> netlink_parse_response);
> }
>
> +#include "socketutils.h"
> /* Given an inode number of a socket, print out the details
> * of the ip address and port. */
> bool
> -print_sockaddr_by_inode(const unsigned long inode, const char *const
> proto_name)
> +print_sockaddr_by_inode(const unsigned long inode, const int proto)
> {
> - static const struct {
> - const char *const name;
> - bool (*const print)(int, unsigned long);
> - } protocols[] = {
> - { "TCP", tcp_v4_print },
> - { "UDP", udp_v4_print },
> - { "TCPv6", tcp_v6_print },
> - { "UDPv6", udp_v6_print },
> - { "UNIX", unix_print },
> - { "NETLINK", netlink_print }
> - };
> -
What's wrong with storing this kind of information in a table?
> const int fd = socket(AF_NETLINK, SOCK_RAW, NETLINK_SOCK_DIAG);
> if (fd < 0)
> return false;
> bool r = false;
> unsigned int i;
>
> - if (proto_name) {
> - for (i = 0; i < ARRAY_SIZE(protocols); ++i) {
> - if (strcmp(proto_name, protocols[i].name) == 0) {
> - r = protocols[i].print(fd, inode);
> - break;
> - }
> + if (proto != -1) {
> + r = true;
> +
> + switch (proto) {
> + case TCP:
> + r = tcp_v4_print(fd, inode);
> + break;
> + case UDP:
> + r = udp_v4_print(fd, inode);
> + break;
> + case TCPv6:
> + r = tcp_v6_print(fd, inode);
> + break;
> + case UDPv6:
> + r = udp_v6_print(fd, inode);
> + break;
> + case UNIX:
> + r = unix_print(fd, inode);
> + break;
> + case NETLINK:
> + r = netlink_print(fd, inode);
> + break;
> + default:
> + r = false;
> }
Is this switch statement better than a table?
>
> if (!r) {
> - tprintf("%s:[%lu]", proto_name, inode);
> + tprintf("%s:[%lu]",
> + socket_protocol_strings[proto],
> + inode);
If proto is not one of these 6 enums, this would result to read out of array
bounds.
> r = true;
> }
> } else {
> - for (i = 0; i < ARRAY_SIZE(protocols); ++i) {
> - if ((r = protocols[i].print(fd, inode)))
> +
> + bool (*const protocols_print[])(int, unsigned long) =
> + {
> + tcp_v4_print, udp_v4_print,
> + tcp_v6_print, udp_v6_print,
> + unix_print, netlink_print
> + };
If you've ended up with a table, why not just have a table?
For example,
/* defs.h */
enum sock_proto {
SOCK_PROTO_UNKNOWN,
SOCK_PROTO_UNIX,
SOCK_PROTO_TCP,
SOCK_PROTO_UDP,
SOCK_PROTO_TCPv6,
SOCK_PROTO_UDPv6,
SOCK_PROTO_NETLINK
};
extern bool print_sockaddr_by_inode(const unsigned long, const enum sock_proto);
/* socketutils.c */
static const struct {
const char *const name;
bool (*const print)(int, unsigned long);
} protocols[] = {
[SOCK_PROTO_UNIX] = { "UNIX", unix_print },
[SOCK_PROTO_TCP] = { "TCP", tcp_v4_print },
[SOCK_PROTO_UDP] = { "UDP", udp_v4_print },
[SOCK_PROTO_TCPv6] = { "TCPv6", tcp_v6_print },
[SOCK_PROTO_UDPv6] = { "UDPv6", udp_v6_print },
[SOCK_PROTO_NETLINK] = { "NETLINK", netlink_print }
};
print_sockaddr_by_inode(const unsigned long inode,
const enum sock_proto proto)
{
if ((unsigned int) proto >= ARRAY_SIZE(protocols) ||
(proto != SOCK_PROTO_UNKNOWN && !protocols[proto].print))
return false;
const int fd = socket(AF_NETLINK, SOCK_RAW, NETLINK_SOCK_DIAG);
if (fd < 0)
return false;
bool r = false;
if (proto != SOCK_PROTO_UNKNOWN) {
r = protocols[proto].print(fd, inode);
if (!r) {
tprintf("%s:[%lu]", protocols[proto].name, inode);
r = true;
}
} else {
unsigned int i;
for (i = (unsigned int) SOCK_PROTO_UNKNOWN + 1;
i < ARRAY_SIZE(protocols); ++i) {
if (!protocols[i].print)
continue;
r = protocols[i].print(fd, inode);
if (r)
break;
}
}
close(fd);
return r;
}
> +#ifndef SOCKETUTILS_H
> +# define SOCKETUTILS_H
> +
> +enum {
> + NETLINK = 0,
> + TCP = 1,
> + TCPv6,
> + UDP,
> + UDPv6,
> + UNIX
These names are too short, let's use something that's less likely to collide.
> +} socket_protocols;
Here a variable is being created, which is probably not what you want.
> +extern const char *socket_protocol_strings[];
Since users of this array cannot ensure that out-of-bounds access doesn't
happen, I don't think exporing this internal array is a good idea.
Let's define a function that takes a string and returns the
corresponding enum value.
For example,
enum sock_proto
get_proto_by_name(const char *const name)
{
unsigned int i;
for (i = (unsigned int) SOCK_PROTO_UNKNOWN + 1;
i < ARRAY_SIZE(protocols); ++i) {
if (protocols[i].name && !strcmp(name, protocols[i].name))
return (enum sock_proto) i;
}
return SOCK_PROTO_UNKNOWN;
}
--
ldv
pgpzMdhcODqZc.pgp
Description: PGP signature
------------------------------------------------------------------------------ What NetFlow Analyzer can do for you? Monitors network bandwidth and traffic patterns at an interface-level. Reveals which users, apps, and protocols are consuming the most bandwidth. Provides multi-vendor support for NetFlow, J-Flow, sFlow and other flows. Make informed decisions using capacity planning reports. https://ad.doubleclick.net/ddm/clk/305295220;132659582;e
_______________________________________________ Strace-devel mailing list Strace-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/strace-devel
