[SCM] strace branch, master, updated. v4.6-128-g2fb4db3
Dmitry V. Levin
ldv at altlinux.org
Wed Aug 31 20:33:15 UTC 2011
On Wed, Aug 31, 2011 at 10:32:55AM +0000, Denys Vlasenko wrote:
> commit 2fb4db3e7aa1d6ac6b4b43f47597197492a846dd
> Author: Denys Vlasenko <dvlasenk at redhat.com>
> Date: Wed Aug 31 12:26:03 2011 +0200
>
> Optimization: eliminate all remaining usages of strcat()
>
> After this change, we don't use strcat() anywhere.
>
> * defs.h: Change sprinttv() return type to char *.
> * time.c (sprinttv): Return pointer past last stored char.
> * desc.c (decode_select): Change printing logic in order to eliminate
> usage of strcat() - use stpcpy(), *outptr++ = ch, sprintf() instead.
> Also reduce usage of strlen().
> * stream.c (decode_poll): Likewise.
The idea behind this change is quite clear, but some hunks are questionable.
> --- a/desc.c
> +++ b/desc.c
> @@ -493,7 +493,6 @@ decode_select(struct tcb *tcp, long *args, enum bitness_t bitness)
> unsigned int fdsize = ((((args[0] + 7) / 8) + sizeof(long) - 1)
> & -sizeof(long));
> fd_set *fds;
> - static char outstr[1024];
> const char *sep;
> long arg;
>
> @@ -532,8 +531,10 @@ decode_select(struct tcb *tcp, long *args, enum bitness_t bitness)
> printtv_bitness(tcp, args[4], bitness, 0);
> }
> else {
> - unsigned int cumlen = 0;
> - const char *sep = "";
> + static char outstr[1024];
> + char *outptr;
> +#define end_outstr (outstr + sizeof(outstr))
> + const char *sep;
>
> if (syserror(tcp))
> return 0;
> @@ -544,41 +545,42 @@ decode_select(struct tcb *tcp, long *args, enum bitness_t bitness)
> return RVAL_STR;
> }
>
> - fds = (fd_set *) malloc(fdsize);
> + fds = malloc(fdsize);
> if (fds == NULL)
> fprintf(stderr, "out of memory\n");
>
> - outstr[0] = '\0';
> + tcp->auxstr = outstr;
I suggest to initialize tcp->auxstr as late as possible (just in case
there is a branch of code that might fail to initialize it properly),
better right before returning RVAL_STR.
> + outptr = outstr;
> + sep = "";
> for (i = 0; i < 3; i++) {
> int first = 1;
>
> - tcp->auxstr = outstr;
> arg = args[i+1];
> if (fds == NULL || !arg ||
> umoven(tcp, arg, fdsize, (char *) fds) < 0)
> continue;
> for (j = 0; j < args[0]; j++) {
> if (FD_ISSET(j, fds)) {
> - char str[11 + 3 * sizeof(int)];
> -
> - if (first) {
> - sprintf(str, "%s%s [%u", sep,
> - i == 0 ? "in" :
> - i == 1 ? "out" :
> - "except", j);
> - first = 0;
> - sep = ", ";
> + /* +2 chars needed at the end: ']',NUL */
> + if (outptr < end_outstr - (sizeof(", except [") + sizeof(int)*3 + 2)) {
> + if (first) {
> + outptr += sprintf(outptr, "%s%s [%u",
> + sep,
> + i == 0 ? "in" : i == 1 ? "out" : "except",
> + j
> + );
> + first = 0;
> + sep = ", ";
> + }
> + else {
> + outptr += sprintf(outptr, " %u", j);
> + }
> }
> - else
> - sprintf(str, " %u", j);
I'm not sure about this change. The new code is less readable and
requires more room in outstr[] than the old code.
> - cumlen += strlen(str);
> - if (cumlen < sizeof(outstr))
> - strcat(outstr, str);
> nfds--;
> }
> }
> - if (cumlen)
> - strcat(outstr, "]");
> + if (outptr != outstr)
> + *outptr++ = ']';
> if (nfds == 0)
> break;
> }
> @@ -586,15 +588,15 @@ decode_select(struct tcb *tcp, long *args, enum bitness_t bitness)
> #ifdef LINUX
> /* This contains no useful information on SunOS. */
> if (args[4]) {
> - char str[128];
> -
> - sprintf(str, "%sleft ", sep);
> - sprinttv(tcp, args[4], bitness, str + strlen(str));
> - if ((cumlen += strlen(str)) < sizeof(outstr))
> - strcat(outstr, str);
> + if (outptr < end_outstr - 128) {
> + outptr += sprintf(outptr, "%sleft ", sep);
> + outptr = sprinttv(tcp, args[4], bitness, outptr);
> + }
The new code requires more room in outstr[] than the old code.
> #endif /* LINUX */
> + *outptr = '\0';
> return RVAL_STR;
> +#undef end_outstr
> }
> return 0;
> }
> diff --git a/stream.c b/stream.c
> index d55beb5..165e116 100644
> --- a/stream.c
> +++ b/stream.c
> @@ -341,9 +341,9 @@ decode_poll(struct tcb *tcp, long pts)
> return 0;
> } else {
> static char outstr[1024];
> - char str[64];
> + char *outptr;
> +#define end_outstr (outstr + sizeof(outstr))
> const char *flagstr;
> - unsigned int cumlen;
>
> if (syserror(tcp))
> return 0;
> @@ -366,62 +366,56 @@ decode_poll(struct tcb *tcp, long pts)
> abbrev_end = end;
> }
>
> - outstr[0] = '\0';
> - cumlen = 0;
> + outptr = outstr;
>
> for (cur = start; cur < end; cur += sizeof(fds)) {
> if (umoven(tcp, cur, sizeof fds, (char *) &fds) < 0) {
> - ++cumlen;
> - if (cumlen < sizeof(outstr))
> - strcat(outstr, "?");
> + if (outptr < end_outstr - 2)
> + *outptr++ = '?';
> failed = 1;
> break;
> }
> if (!fds.revents)
> continue;
> - if (!cumlen) {
> - ++cumlen;
> - strcat(outstr, "[");
> + if (outptr == outstr) {
> + *outptr++ = '[';
> } else {
> - cumlen += 2;
> - if (cumlen < sizeof(outstr))
> - strcat(outstr, ", ");
> + if (outptr < end_outstr - 3)
> + outptr = stpcpy(outptr, ", ");
> }
> if (cur >= abbrev_end) {
> - cumlen += 3;
> - if (cumlen < sizeof(outstr))
> - strcat(outstr, "...");
> + if (outptr < end_outstr - 4)
> + outptr = stpcpy(outptr, "...");
> break;
> }
> - sprintf(str, "{fd=%d, revents=", fds.fd);
> + if (outptr < end_outstr - (sizeof("{fd=%d, revents=") + sizeof(int)*3) + 1)
> + outptr += sprintf(outptr, "{fd=%d, revents=", fds.fd);
> flagstr = sprintflags("", pollflags, fds.revents);
> - cumlen += strlen(str) + strlen(flagstr) + 1;
> - if (cumlen < sizeof(outstr)) {
> - strcat(outstr, str);
> - strcat(outstr, flagstr);
> - strcat(outstr, "}");
> + if (outptr < end_outstr - (strlen(flagstr) + 2)) {
> + outptr = stpcpy(outptr, flagstr);
> + *outptr++ = '}';
The new code requires more room in outstr[] than the old code.
> }
> }
> if (failed)
> return 0;
>
> - if (cumlen && ++cumlen < sizeof(outstr))
> - strcat(outstr, "]");
> + if (outptr != outstr /* && outptr < end_outstr - 1 (always true)*/)
> + *outptr++ = ']';
>
> + *outptr = '\0';
> if (pts) {
> - char str[128];
> -
> - sprintf(str, "%sleft ", cumlen ? ", " : "");
> - sprint_timespec(str + strlen(str), tcp, pts);
> - if ((cumlen += strlen(str)) < sizeof(outstr))
> - strcat(outstr, str);
> + if (outptr < end_outstr - 128) {
> + outptr = stpcpy(outptr, outptr == outstr ? "left " : ", left ");
> + sprint_timespec(outptr, tcp, pts);
> + }
The new code requires more room in outstr[] than the old code.
> }
>
> - if (!outstr[0])
> + if (outptr == outstr)
> return 0;
>
> tcp->auxstr = outstr;
> return RVAL_STR;
> +#undef end_outstr
> }
> }
>
> diff --git a/time.c b/time.c
> index a4599f7..89a6216 100644
> --- a/time.c
> +++ b/time.c
> @@ -112,40 +112,44 @@ printtv_bitness(struct tcb *tcp, long addr, enum bitness_t bitness, int special)
> }
> }
>
> -void
> +char *
> sprinttv(struct tcb *tcp, long addr, enum bitness_t bitness, char *buf)
> {
> + int rc;
> +
> if (addr == 0)
> - strcpy(buf, "NULL");
> - else if (!verbose(tcp))
> - sprintf(buf, "%#lx", addr);
> - else {
> - int rc;
> + return stpcpy(buf, "NULL");
>
> - if (bitness == BITNESS_32
> + if (!verbose(tcp)) {
> + buf += sprintf(buf, "%#lx", addr);
> + return buf;
> + }
Why not just write it simply
if (!verbose(tcp))
return buf + sprintf(buf, "%#lx", addr);
> +
> + if (bitness == BITNESS_32
> #if defined(LINUX) && SUPPORTED_PERSONALITIES > 1
> - || personality_wordsize[current_personality] == 4
> + || personality_wordsize[current_personality] == 4
> #endif
> - )
> - {
> - struct timeval32 tv;
> -
> - rc = umove(tcp, addr, &tv);
> - if (rc >= 0)
> - sprintf(buf, "{%u, %u}",
> - tv.tv_sec, tv.tv_usec);
> - } else {
> - struct timeval tv;
> + )
> + {
> + struct timeval32 tv;
> +
> + rc = umove(tcp, addr, &tv);
> + if (rc >= 0)
> + buf += sprintf(buf, "{%u, %u}",
> + tv.tv_sec, tv.tv_usec);
Why not just return buf + sprintf here,
> + } else {
> + struct timeval tv;
>
> - rc = umove(tcp, addr, &tv);
> - if (rc >= 0)
> - sprintf(buf, "{%lu, %lu}",
> - (unsigned long) tv.tv_sec,
> - (unsigned long) tv.tv_usec);
> - }
> - if (rc < 0)
> - strcpy(buf, "{...}");
> + rc = umove(tcp, addr, &tv);
> + if (rc >= 0)
> + buf += sprintf(buf, "{%lu, %lu}",
> + (unsigned long) tv.tv_sec,
> + (unsigned long) tv.tv_usec);
here and
> }
> + if (rc < 0)
> + buf = stpcpy(buf, "{...}");
here?
> +
> + return buf;
> }
>
> void print_timespec(struct tcb *tcp, long addr)
--
ldv
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: not available
URL: <http://lists.strace.io/pipermail/strace-devel/attachments/20110901/c43e1d45/attachment.bin>
More information about the Strace-devel
mailing list