Re: Comments on the libmemcached (updated) patch?
On Thu, 2011-10-06 at 16:58:47 +0200, Svante Signell wrote:
> 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)))
> {
I don't think this is right, if there's no IOV_MAX limit then there's no
reason to start a new msghdr on that specific condition.
> 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 */
I've not checked the context, but this seems like a waste of resources.
If the string is already being allocated then don't strdup() and free()
it, just assign it to ms_setting.cfg_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);
You need to free buffer here, or it's going to leak once you overwrite
it with the next malloc()s.
> 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);
This conditional is not indented properly.
> + 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);
Indentation.
> + 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))
> @@ -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 */
Small nitpick, space missing before NULL.
regards,
guillem
Reply to: