Re: Comments on the libmemcached (updated) patch?
On Wed, 2011-09-28 at 22:15 +0200, Neal H. Walfield wrote:
> > For this patch, the amount of data dynamically allocated is not large, a
> > few strings of size much less than PATH_MAX of 4096 bytes. According to
> > the manpage for malloc the default MMAP_THRESHOLD is at 128kB.
> >
> > Alternately asprintf could be used instead of malloc+snprintf, but
> > asprintf is not standardized in C or POSIX (yet?). Additionally, it also
> > allocates memory from the heap??
>
> It looks like you could use alloca to allocate the memory. This would
> actually improve the performance of the original code, because a more
> compact stack will result in a hotter stack.
Thanks for the hint. However, I did not use alloca since it might have
side effects according to the manpage (and it's not in POSIX.1-2001)
Attached is a new patch for review.
For clarity:
#define DEFAULT_CONFIG_NAME ".memslap.cnf"
#define PID_FILE_BASE "/tmp/%ulibmemcached_memc.pid"
#define MEMCACHED_BINARY "$MEMC_BINARY"
MEMC_BINARY='/usr/bin/memcached'
Thanks!
diff -ur libmemcached-0.44/clients/ms_conn.c libmemcached-0.44.modified//clients/ms_conn.c
--- libmemcached-0.44/clients/ms_conn.c 2010-07-22 17:06:58.000000000 +0200
+++ libmemcached-0.44.modified//clients/ms_conn.c 2011-09-26 21:47:14.000000000 +0200
@@ -2125,6 +2125,9 @@
limit_to_mtu= c->udp;
/* We may need to start a new msghdr if this one is full. */
+#ifndef IOV_MAX
+#define IOV_MAX 1024
+#endif
if ((m->msg_iovlen == IOV_MAX)
|| (limit_to_mtu && (c->msgbytes >= UDP_MAX_SEND_PAYLOAD_SIZE)))
{
diff -ur libmemcached-0.44/clients/ms_setting.c libmemcached-0.44.modified//clients/ms_setting.c
--- libmemcached-0.44/clients/ms_setting.c 2010-08-03 02:34:02.000000000 +0200
+++ libmemcached-0.44.modified//clients/ms_setting.c 2011-10-05 18:17:52.000000000 +0200
@@ -304,13 +304,17 @@
*/
static void ms_no_config_file()
{
- char userpath[PATH_MAX];
+ char *userpath= NULL;
+ size_t len;
struct passwd *usr= NULL;
FILE *fd;
usr= getpwuid(getuid());
- snprintf(userpath, PATH_MAX, "%s/%s", usr->pw_dir, DEFAULT_CONFIG_NAME);
+ len= strlen(usr->pw_dir) + 1 + strlen(DEFAULT_CONFIG_NAME) + 1;
+ if ((userpath = malloc(len)) == NULL)
+ exit(1);
+ snprintf(userpath, len, "%s/%s", usr->pw_dir, DEFAULT_CONFIG_NAME);
if (access (userpath, F_OK | R_OK) == 0)
goto exit;
@@ -321,6 +325,7 @@
{
fprintf(stderr, "Could not create default configure file %s\n", userpath);
perror(strerror(errno));
+ free(userpath);
exit(1);
}
fprintf(fd, "%s", DEFAULT_CONGIF_STR);
@@ -328,6 +333,7 @@
exit:
ms_setting.cfg_file= strdup(userpath);
+ free(userpath);
} /* ms_no_config_file */
--- libmemcached-0.44/tests/server.c 2010-08-03 08:30:54.000000000 +0200
+++ libmemcached-0.44.modified/tests/server.c 2011-10-05 19:08:08.000000000 +0200
@@ -117,18 +117,30 @@
}
}
- char buffer[PATH_MAX];
- snprintf(buffer, sizeof(buffer), PID_FILE_BASE, x);
+ char *buffer= NULL;
+ size_t len= 0;
+
+ len= strlen(PID_FILE_BASE) + sizeof(int) - 2 + 1;
+ if ((buffer = malloc(len)) == NULL)
+ assert(buffer);
+ snprintf(buffer, len, PID_FILE_BASE, x);
kill_file(buffer);
if (x == 0)
{
- snprintf(buffer, sizeof(buffer), "%s -d -u root -P "PID_FILE_BASE" -t 1 -p %u -U %u -m 128",
+ len= strlen("%s -d -u root -P "PID_FILE_BASE" -t 1 -p %u -U %u -m 128") + strlen(MEMCACHED_BINARY) + 3*sizeof(int) - 4*2 + 1;
+ if ((buffer = malloc(len)) == NULL)
+ assert(buffer);
+ snprintf(buffer, len, "%s -d -u root -P "PID_FILE_BASE" -t 1 -p %u -U %u -m 128",
MEMCACHED_BINARY, x, port, port);
}
else
{
- snprintf(buffer, sizeof(buffer), "%s -d -u root -P "PID_FILE_BASE" -t 1 -p %u -U %u",
+ len= strlen("%s -d -u root -P "PID_FILE_BASE" -t 1 -p %u -U %u") +
+ strlen(MEMCACHED_BINARY) + 3*sizeof(int) - 4*2 + 1;
+ if ((buffer = malloc(len)) == NULL)
+ assert(buffer);
+ snprintf(buffer, len, "%s -d -u root -P "PID_FILE_BASE" -t 1 -p %u -U %u",
MEMCACHED_BINARY, x, port, port);
}
if (libmemcached_util_ping("localhost", port, NULL))
@@ -142,6 +154,7 @@
}
count= sprintf(end_ptr, "localhost:%u,", port);
end_ptr+= count;
+ free(buffer);
}
*end_ptr= 0;
@@ -149,9 +162,13 @@
int *pids= calloc(construct->count, sizeof(int));
for (uint32_t x= 0; x < construct->count; x++)
{
- char buffer[PATH_MAX]; /* Nothing special for number */
+ char *buffer= NULL; /* Nothing special for number */
+ size_t len= 0;
- snprintf(buffer, sizeof(buffer), PID_FILE_BASE, x);
+ len= strlen(PID_FILE_BASE) + sizeof(int) - 2 + 1;
+ if ((buffer= malloc(len)) == NULL)
+ assert(buffer);
+ snprintf(buffer, len, PID_FILE_BASE, x);
uint32_t counter= 3000; // Absurd, just to catch run away process
while (pids[x] <= 0 && --counter)
@@ -198,8 +215,10 @@
if (pids[y] > 0)
kill(pids[y], SIGTERM);
}
+ free(buffer);
abort();
}
+ free(buffer);
}
free(pids);
@@ -229,9 +248,15 @@
{
for (uint32_t x= 0; x < construct->count; x++)
{
- char file_buffer[PATH_MAX]; /* Nothing special for number */
- snprintf(file_buffer, sizeof(file_buffer), PID_FILE_BASE, x);
+ char *file_buffer=NULL; /* Nothing special for number */
+ size_t len= 0;
+
+ len= strlen(PID_FILE_BASE) + sizeof(int) - 2 + 1;
+ if ((file_buffer = malloc(len)) == NULL)
+ assert(file_buffer);
+ snprintf(file_buffer, len, PID_FILE_BASE, x);
kill_file(file_buffer);
+ free(file_buffer);
}
free(construct->server_list);
Reply to: