[PATCH 1/3] Introduce socket address information caching(scache) layer

[Mike Frysinger]

On 20 Mar 2015 01:17, Masatake YAMATO wrote:
> --- a/socketutils.c
> +++ b/socketutils.c
> @@ -7,6 +7,7 @@
>  #include <linux/inet_diag.h>
>  #include <linux/unix_diag.h>
>  #include <linux/rtnetlink.h>
> +#include <assert.h>

shouldn't all non-linux/asm headers be before linux/asm includes ?

> +#define ENTRY(e) ((struct scache_entry*)(e))

prob want a space before the *

> +struct scache_protocol {
> +	const char *name;			     /* Must */
> +	size_t size;				     /* Must */
> +	void (* print) (struct scache_entry *entry); /* Must */

no space before the (

> +static struct scache_entry *
> +scache_entry_init(struct scache_entry *entry,
> +		  unsigned long inode,
> +		  struct scache_protocol *protocol)
> +{
> +	memset(entry, 0, protocol->size);
> +	entry->inode = inode;
> +	entry->protocol = protocol;
> +	return entry;
> +}
> +
> +static void
> +scach_entry_free(struct scache_entry *entry)

typo -- "scache"

> +{
> +	free (entry);
> +}

no space before the (

> +/* Hashtable keyed by inode number */

periods at the end of sentences

> +#define SCACHE_ENTRIES_SIZE 43
> +static struct scache_entry *scache_entries [SCACHE_ENTRIES_SIZE];

no space before the [

> +/* Least Recenly Used queue
> + * If strace records all established sockets, the cache will overflow.
> + * With this LRU queue, unused sockets can be thrown away.
> + */

would trying to have a general hash map/lru queue/linked list be worth the code 
overhead ?  glib and such seems to pull it off.  seems crazy & error prone to 
have to implement these inline everytime we want these datastructures ...

> +	if ((*chain) == entry) {

no need for those inner parens

> +	e = malloc(s);
> +	if (!e)
> +		die_out_of_memory();

not a new issue, but we probably could do with a xmalloc helper

> +	return r? true: false;

space before the r, or just return !!r

> --- a/util.c
> +++ b/util.c
>  
> +static bool
> +printsockinode(struct tcb *tcp, int fd, unsigned long inodenr)
> +{
> +	if (print_sockaddr_in_cache(inodenr))
> +		return true;
> +	else {
> +#define PROTO_NAME_LEN 32
> +		char proto_buf[PROTO_NAME_LEN];
> +		const char *proto;
> +
> +		proto = getfdproto(tcp, fd, proto_buf, PROTO_NAME_LEN);

seems like you just use sizeof() and drop PROTO_NAME_LEN
-mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.strace.io/pipermail/strace-devel/attachments/20150319/05ae276b/attachment.bin>