5
\$\begingroup\$

I have the following code that takes user input "path" and adds the current time into the filename after the name part and before any extension (if it exists). It seems a little hacky to me just looking at it. What do you think?

#include <string.h>
#include <stdio.h>
#include <libgen.h>

int main() {
    char buf1[256] = {0};
    char path[] = "/var/log/mine.log";
    char *tmp = NULL;
    char time[] = "20201110120305";
    char *dir = NULL;
    char *base = NULL;
    char *file = NULL;
    char *ext = NULL;
    //split path to base and dir
    tmp = strdup(path);
    dir = dirname(tmp);
    tmp = strdup(path);
    base = basename(tmp);
    //split base to file and ext
    tmp = strchr(base,'.');
    if (tmp) {
        ext = strdup(tmp);
        file = strndup(base,(strlen(base)-strlen(ext)));
    }
    else
        file = strdup(base);
    // concat dir / file / time / ext
    if (dir) 
        strcat(buf1,dir);
    strcat(buf1,"/");
    if (file) 
        strcat(buf1,file);
    strcat(buf1,"-");
    if (time)
        strcat(buf1,time);
    if (ext)
        strcat(buf1,ext);
    printf("%s\n",buf1);
}

This will output /var/log/mine-20201110120305.log

\$\endgroup\$
1
  • 1
    \$\begingroup\$ I've added the posix tag, as I don't think that strdup() and <libgen.h> are generally available on other platforms. \$\endgroup\$ – Toby Speight Nov 10 '20 at 13:52
3
\$\begingroup\$

Okay, you're doing a lot of [unnecessary] strdup. If we put all the code into a reusable function of its own, you don't free any of the strings, so you are leaking memory.

One way to alleviate that is to use strdupa instead of strdup. This does an alloca to get a stack frame allocation, so these strings don't have to be freed [they disappear with the stack frame when the function returns].

In fact, with better use of pointers, there's really no need to duplicate these intermediate strings at all.

And, there's still a lot of extra scanning/rescanning and strcpy/strcat.

Stylistically, tmp isn't too descriptive. And, it gets reused for two different purposes.


I've refactored your program to put the code into functions. I've added a test framework. And, I've created three additional versions that show what I would do to simplify and speed things up:

// logtod.c -- add timestamp to log file name

#define _GNU_SOURCE
#include <string.h>
#include <stdio.h>
#include <stdlib.h>
#include <libgen.h>
#include <stddef.h>
#include <time.h>

int opt_e;
int opt_n;

time_t stamptod;                        // previous time
int stamplen;                           // length of stampbuf
char stampbuf[100];                     // timestamp string

// todget -- generate timestamp
// RETURNS: stamp in stampbuf and length in stamplen
void
todget(time_t tod)
{
    struct tm *tm;

    if (tod == 0)
        tod = time(NULL);

    do {
        // use cached buffer [if possible]
        if (tod == stamptod)
            break;
        stamptod = tod;

        tm = localtime(&tod);

        stamplen = sprintf(stampbuf,"-%4.4d%2.2d%2.2d%2.2d%2.2d%2.2d",
            tm->tm_year + 1900,tm->tm_mon + 1,tm->tm_mday,
            tm->tm_hour,tm->tm_min,tm->tm_sec);
    } while (0);
}

// logof_orig -- original code [leaks memory]
char *
logof_orig(char *buf1,const char *path,time_t tod)
{
    char *tmp = NULL;
    char *dir = NULL;
    char *base = NULL;
    char *file = NULL;
    char *ext = NULL;

    // get timestamp
    todget(tod);

    // split path to base and dir
    tmp = strdup(path);
    dir = dirname(tmp);
    tmp = strdup(path);
    base = basename(tmp);

    // split base to file and ext
    tmp = strchr(base,'.');
    if (tmp) {
        ext = strdup(tmp);
        file = strndup(base,strlen(base) - strlen(ext));
    }
    else
        file = strdup(base);

    buf1[0] = 0;

    // concat dir / file / time / ext
    if (dir)
        strcat(buf1,dir);
    strcat(buf1,"/");
    if (file)
        strcat(buf1,file);
    strcat(buf1,stampbuf);
    if (ext)
        strcat(buf1,ext);

    return buf1;
}

// logof_alloca -- original code [using strdupa et. al]
char *
logof_alloca(char *buf1,const char *path,time_t tod)
{
    //char buf1[1024] = { 0 };
    char *tmp = NULL;
    char *dir = NULL;
    char *base = NULL;
    char *file = NULL;
    char *ext = NULL;

    // get timestamp
    todget(tod);

    // split path to base and dir
    tmp = strdupa(path);
    dir = dirname(tmp);
    tmp = strdupa(path);
    base = basename(tmp);

    // split base to file and ext
    tmp = strchr(base,'.');
    if (tmp) {
        ext = strdupa(tmp);
        file = strndupa(base,strlen(base) - strlen(ext));
    }
    else
        file = strdupa(base);

    buf1[0] = 0;

    // concat dir / file / time / ext
    if (dir)
        strcat(buf1,dir);
    strcat(buf1,"/");
    if (file)
        strcat(buf1,file);
    strcat(buf1,stampbuf);
    if (ext)
        strcat(buf1,ext);

    return buf1;
}

// logof_stpcpy -- original code [using stpcpy vs. strcat]
char *
logof_stpcpy(char *buf1,const char *path,time_t tod)
{
    char *tmp = NULL;
    char *dir = NULL;
    char *base = NULL;
    char *file = NULL;
    char *ext = NULL;

    // get timestamp
    todget(tod);

    // split path to base and dir
    tmp = strdupa(path);
    dir = dirname(tmp);
    tmp = strdupa(path);
    base = basename(tmp);

    // split base to file and ext
    tmp = strchr(base,'.');
    if (tmp) {
        ext = strdupa(tmp);
        file = strndupa(base,strlen(base) - strlen(ext));
    }
    else
        file = strdupa(base);

    buf1[0] = 0;

    // concat dir / file / time / ext
    char *dst = buf1;
    if (dir)
        dst = stpcpy(dst,dir);
    dst = stpcpy(dst,"/");
    if (file)
        dst = stpcpy(dst,file);
    dst = stpcpy(dst,stampbuf);
    if (ext)
        dst = stpcpy(dst,ext);

    return buf1;
}

// copyout -- fill in destination buffer
char *
copyout(char *buf1,const char *path,int pathlen,time_t tod,
        const char *base,const char *ext)
{
    char *dst;
    ptrdiff_t cpylen;

    // get timestamp
    todget(tod);

    // get enough buffer space
    if (buf1 == NULL)
        buf1 = malloc(pathlen + stamplen + 1);

    dst = buf1;

    // ext must be to the right of base
    do {
        if (ext == NULL)
            break;
        if (base == NULL)
            break;
        if (ext >= base)
            break;
        ext = NULL;
    } while (0);

    // copy over all but extension
    if (ext != NULL)
        cpylen = ext - path;
    else
        cpylen = pathlen;
    dst = mempcpy(dst,path,cpylen);

    // copy over the date stamp
    dst = mempcpy(dst,stampbuf,stamplen);

    // copy over the extension
    if (ext != NULL) {
        cpylen = &path[pathlen] - ext;
        dst = mempcpy(dst,ext,cpylen);
    }

    *dst = 0;

    return buf1;
}

// logof_fix1 -- single malloc version
char *
logof_fix1(char *buf1,const char *path,time_t tod)
{

    // get length of path
    size_t pathlen = strlen(path);

    // find "base" in "/dir/base"
    const char *base = strrchr(path,'/');
    if (base != NULL)
        ++base;

    // find the file extension
    const char *ext;
    if (base != NULL)
        ext = strchr(base,'.');
    else
        ext = strrchr(path,'.');

    buf1 = copyout(buf1,path,pathlen,tod,base,ext);

    return buf1;
}

// logof_fix2 -- single pass scan
char *
logof_fix2(char *buf1,const char *path,time_t tod)
{
    const char *ext = NULL;
    const char *base = path;

    // find "base.ext" and ".ext" in "/dir/base.ext"
    const char *src = path;
    for (int chr = *src++;  chr != 0;  chr = *src++) {
        switch (chr) {
        case '/':
            base = src;
            ext = NULL;
            break;
        case '.':
            if (ext == NULL)
                ext = src - 1;
            break;
        }
    }

    // full length of path
    ptrdiff_t pathlen = (src - 1) - path;

    // get enough buffer space
    buf1 = copyout(buf1,path,pathlen,tod,base,ext);

    return buf1;
}

// logof_fix3 -- single pass scan [using strcspn]
char *
logof_fix3(char *buf1,const char *path,time_t tod)
{
    const char *ext = NULL;
    const char *base = path;

    // find "base.ext" and ".ext" in "/dir/base.ext"
    const char *src = path;
    while (1) {
        int len = strcspn(src,"/.");

        // point to start of separater
        src += len;

        // get the character
        int chr = *src;

        // end of string
        if (chr == 0)
            break;

        // separater is at the start of the source string
        if (len == 0) {
            src += 1;
            continue;
        }

        switch (chr) {
        case '/':
            base = src + 1;
            ext = NULL;
            break;
        case '.':
            if (ext == NULL)
                ext = src;
            break;
        }
    }

    // full length of path
    ptrdiff_t pathlen = src - path;

    buf1 = copyout(buf1,path,pathlen,tod,base,ext);

    return buf1;
}

// fixname -- compensensate for dirname changing "foo" into "./foo"
const char *
fixname(const char *out)
{

    if ((out[0] == '.') && (out[1] == '/'))
        out += 2;

    return out;
}

typedef long long tsc_t;

tsc_t
tscget(void)
{
    struct timespec ts;
    tsc_t tsc;

    clock_gettime(CLOCK_MONOTONIC,&ts);
    tsc = ts.tv_sec;
    tsc *= 1000000000;
    tsc += ts.tv_nsec;

    return tsc;
}

double
tscsec(tsc_t tsc)
{
    double sec;

    sec = tsc;
    sec /= 1e9;

    return sec;
}

// test control
typedef char *(*logof_p)(char *buf1,const char *path,time_t tod);
typedef struct {
    logof_p tst_fnc;
    const char *tst_tag;
    const char *tst_reason;
} dotest_t;

#define DOTEST(_fnc,_reason) \
    { .tst_fnc = logof_##_fnc, .tst_tag = #_fnc, .tst_reason = _reason },

// list of tests
dotest_t tstlist[] = {
    DOTEST(orig,"original code [leaks memory]")
    DOTEST(alloca,"original code [using strdupa et. al]")
    DOTEST(stpcpy,"original code [using stpcpy vs. strcat]")
    DOTEST(fix1,"single malloc version")
    DOTEST(fix2,"single pass scan")
    DOTEST(fix3,"single pass scan [using strcspn]")
    { .tst_fnc = NULL }
};

int fnclen;                         // maximum length of a function name
char *dotest_prevbuf;               // previous output buffer
tsc_t dotest_prevtsc;               // best time for previous test
tsc_t dotest_origtsc;               // best time for original test

// dotest_ratio -- show ratio of times
void
dotest_ratio(tsc_t curtsc,tsc_t prevtsc,const char *who)
{
    double ratio;
    const char *tag;

    if (curtsc > prevtsc) {
        tag = "slower";
        ratio = curtsc;
        ratio /= prevtsc;
    }
    else {
        tag = "faster";
        ratio = prevtsc;
        ratio /= curtsc;
    }

    printf(" -- %.3fx %s",ratio,tag);
}

// dotest -- perform test of single function
void
dotest(dotest_t *tst,const char *path)
{
    char *out;
    tsc_t tscbest = 1LL << 62;
    int showout = 0;
    int fail = 0;

    out = malloc(1000);

    // do test multiple times to eliminate variations in caching, speed step,
    // timeslicing, etc. and take the best/minimum time
    for (int iter = 10;  iter > 0;  --iter) {
        tsc_t tscbeg = tscget();

        out = tst->tst_fnc(out,path,stamptod);

        // get elapsed time
        tsc_t tscend = tscget();
        tscend -= tscbeg;

        // get best time
        if (tscend < tscbest)
            tscbest = tscend;
    }

    if (dotest_origtsc == 0)
        dotest_origtsc = tscbest;

    // compare against previous test data
    do {
        if (dotest_prevbuf == NULL) {
            showout = 1;
            break;
        }

        const char *curfix = fixname(out);
        const char *oldfix = fixname(dotest_prevbuf);
        if (strcmp(curfix,oldfix) != 0)
            fail = 1;
    } while (0);

    printf("%.9f %-*s",
        tscsec(tscbest),fnclen,tst->tst_tag);

    // compare against previous test timing
    do {
        if (dotest_prevbuf == NULL)
            break;

        dotest_ratio(tscbest,dotest_origtsc,"orig");
        dotest_ratio(tscbest,dotest_prevtsc,"prev");
    } while (0);

    if (opt_n)
        printf("\n");

    if (showout || fail)
        printf(" %s (%s)",out,fail ? "FAIL" : "PASS");

    //if (! opt_n)
        printf("\n");

    free(dotest_prevbuf);
    dotest_prevbuf = out;
    dotest_prevtsc = tscbest;
}

void
doall(const char *path)
{

    printf("\n");
    printf("PATH: %s\n",path);

    dotest_origtsc = 0;
    for (dotest_t *tst = tstlist;  tst->tst_fnc != NULL;  ++tst)
        dotest(tst,path);

    free(dotest_prevbuf);
    dotest_prevbuf = NULL;
}

int
main(int argc,char **argv)
{

    --argc;
    ++argv;

    for (;  argc > 0;  --argc, ++argv) {
        char *cp = *argv;
        if (*cp != '-')
            break;

        cp += 2;
        switch (cp[-1]) {
        case 'e':
            opt_e = ! opt_e;
            break;
        case 'n':
            opt_n = ! opt_n;
            break;
        }
    }

    // get maximum length of a function name
    for (dotest_t *tst = tstlist;  tst->tst_fnc != NULL;  ++tst) {
        int curlen = strlen(tst->tst_tag);
        if (curlen > fnclen)
            fnclen = curlen;
    }

    // show the function names and their descriptions
    for (dotest_t *tst = tstlist;  tst->tst_fnc != NULL;  ++tst)
        printf("%-*s -- %s\n",fnclen,tst->tst_tag,tst->tst_reason);

    // use the same timestamp for all tests
    todget(0);

    doall("/var/log/mine.log");
    doall("/var/log/mine");
    doall("/var/log.xxx/mine");
    doall("mine.log");
    doall("mine");
    doall("/var/log/mine.log.gz");
    doall("/.log");
    doall("/.");

    printf("complete\n");

    return 0;
}

Note that I did the fix2 version. But, I thought that using strcspn might be faster, so I added fix3.

That's because strcspn [under glibc, at least] is insanely optimized to very wide memory fetches. But, a side effect is that there's some extra table lookup initialization at entry to the function, so the strings should be long enough to overcome that initial deficit.

After looking at the resultant code, I'm thinking that the number of times strcspn has to be called, the extra complexity of the code, would make fix3 [probably] slower and less desirable than fix2


Here's the program output:

orig   -- original code [leaks memory]
alloca -- original code [using strdupa et. al]
stpcpy -- original code [using stpcpy vs. strcat]
fix1   -- single malloc version
fix2   -- single pass scan
fix3   -- single pass scan [using strcspn]

PATH: /var/log/mine.log
0.000000274 orig   /var/log/mine-20201110175328.log (PASS)
0.000000150 alloca -- 1.827x faster -- 1.827x faster
0.000000132 stpcpy -- 2.076x faster -- 1.136x faster
0.000000063 fix1   -- 4.349x faster -- 2.095x faster
0.000000067 fix2   -- 4.090x faster -- 1.063x slower
0.000000137 fix3   -- 2.000x faster -- 2.045x slower

PATH: /var/log/mine
0.000000197 orig   /var/log/mine-20201110175328 (PASS)
0.000000095 alloca -- 2.074x faster -- 2.074x faster
0.000000097 stpcpy -- 2.031x faster -- 1.021x slower
0.000000049 fix1   -- 4.020x faster -- 1.980x faster
0.000000052 fix2   -- 3.788x faster -- 1.061x slower
0.000000093 fix3   -- 2.118x faster -- 1.788x slower

PATH: /var/log.xxx/mine
0.000000219 orig   /var/log.xxx/mine-20201110175328 (PASS)
0.000000104 alloca -- 2.106x faster -- 2.106x faster
0.000000106 stpcpy -- 2.066x faster -- 1.019x slower
0.000000058 fix1   -- 3.776x faster -- 1.828x faster
0.000000062 fix2   -- 3.532x faster -- 1.069x slower
0.000000103 fix3   -- 2.126x faster -- 1.661x slower

PATH: mine.log
0.000000268 orig   ./mine-20201110175328.log (PASS)
0.000000126 alloca -- 2.127x faster -- 2.127x faster
0.000000106 stpcpy -- 2.528x faster -- 1.189x faster
0.000000061 fix1   -- 4.393x faster -- 1.738x faster
0.000000051 fix2   -- 5.255x faster -- 1.196x faster
0.000000071 fix3   -- 3.775x faster -- 1.392x slower

PATH: mine
0.000000208 orig   ./mine-20201110175328 (PASS)
0.000000088 alloca -- 2.364x faster -- 2.364x faster
0.000000089 stpcpy -- 2.337x faster -- 1.011x slower
0.000000056 fix1   -- 3.714x faster -- 1.589x faster
0.000000042 fix2   -- 4.952x faster -- 1.333x faster
0.000000047 fix3   -- 4.426x faster -- 1.119x slower

PATH: /var/log/mine.log.gz
0.000000278 orig   /var/log/mine-20201110175328.log.gz (PASS)
0.000000154 alloca -- 1.805x faster -- 1.805x faster
0.000000132 stpcpy -- 2.106x faster -- 1.167x faster
0.000000062 fix1   -- 4.484x faster -- 2.129x faster
0.000000071 fix2   -- 3.915x faster -- 1.145x slower
0.000000139 fix3   -- 2.000x faster -- 1.958x slower

PATH: /.log
0.000000256 orig   //-20201110175328.log (PASS)
0.000000134 alloca -- 1.910x faster -- 1.910x faster
0.000000110 stpcpy -- 2.327x faster -- 1.218x faster
0.000000058 fix1   -- 4.414x faster -- 1.897x faster /-20201110175328.log (FAIL)
0.000000047 fix2   -- 5.447x faster -- 1.234x faster
0.000000063 fix3   -- 4.063x faster -- 1.340x slower /.log-20201110175328 (FAIL)

PATH: /.
0.000000254 orig   //-20201110175328. (PASS)
0.000000134 alloca -- 1.896x faster -- 1.896x faster
0.000000109 stpcpy -- 2.330x faster -- 1.229x faster
0.000000053 fix1   -- 4.792x faster -- 2.057x faster /-20201110175328. (FAIL)
0.000000042 fix2   -- 6.048x faster -- 1.262x faster
0.000000063 fix3   -- 4.032x faster -- 1.500x slower /.-20201110175328 (FAIL)
complete

UPDATE:

I see your point about too much strdup(), but it's necessary with dirname() and basename() as they will mangle their argument.

As I mentioned, raw strdup leaks memory. And, even with dirname/basename some of the dups aren't needed.

And, I've never used either dirname and/or basename for pretty much the same reason [they modify their arguments]. So, I've written my own variants that copy the results to a buffer selected from a pool of static buffers

I like the approach you took with fix1 as it is still very obvious what is going on.

I intended to provide a progression from simple/slow to faster, so you can choose.

Although fix2 is arguably faster, it's a little obfuscated

Perhaps, but it's actually [loosely] derived from production code I have for doing the same thing. You wanted [some] comments. Personally, I felt your code was difficult to follow.

and would not handle extensions such as .log.gz correctly

That wasn't part of your original test data. But, my production code handled it, just not what I recreated for this post. It's a three line fix:

diff --git a/logtod/logtod.c b/logtod/logtod.c
index 28f5702..c212ba9 100644
--- a/logtod/logtod.c
+++ b/logtod/logtod.c
@@ -264,9 +264,11 @@ logof_fix2(char *buf1,const char *path,time_t tod)
        switch (chr) {
        case '/':
            base = src;
+           ext = NULL;
            break;
        case '.':
-           ext = src - 1;
+           if (ext == NULL)
+               ext = src - 1;
            break;
        }
    }

I've updated the original code block and the output


UPDATE #2:

I've updated the code and program output [again ;-)].

I moved the setting of the timestamp buffer to outside the benchmarked/timed area to [better] compare just the times for the scanning/output code (e.g. because sprintf is somewhat heavyweight and took up the bulk of the time spent).

And, I replaced a strlen call in copyout with just a pointer/offset calculation.

And, I added calculation of the speed ratio of the various methods against one another.

\$\endgroup\$
3
  • 1
    \$\begingroup\$ Although strdup() is specified by POSIX, strdupa() is less widely available (GNU only, I think). \$\endgroup\$ – Toby Speight Nov 10 '20 at 13:49
  • 1
    \$\begingroup\$ @TobySpeight When I was first forming the answer, I started with a macro I wrote [have had for 10+ years] that did the same. I was looking at the man page for strdup [for reference] and found strdupa. The man page defines it as a function but, I was curious since I thought it had to be a macro [and it was]. It's easy to create it. Anyway, it's only an interim step as I mentioned because the [massive] duping isn't necessarily the best approach. \$\endgroup\$ – Craig Estey Nov 10 '20 at 14:15
  • \$\begingroup\$ @CraigEstey I see your point about too much strdup(), but it's necessary with dirname() and basename() as they will mangle their argument. I like the approach you took with fix1 as it is still very obvious what is going on. Although fix2 is arguably faster, it's a little obfuscated and would not handle extensions such as .log.gz correctly. \$\endgroup\$ – Jacob Geigle Nov 10 '20 at 15:30
2
\$\begingroup\$

Structure

This doesn't look like a completed program - everything in main() with hard-coded values. A simpler version of a program that does the same thing is:

#include <stdio.h>

int main(void)
{
     puts("/var/log/mine-20201110120305.log");
}

We probably want to accept an argument, and use a function. I'd recommend writing the function with a similar signature to snprintf() and similar - accept a buffer pointer and size, and return the number of characters which would be written if the size were large enough. Whilst not always ideal, this doesn't impose memory allocation on programs that don't need it, and the interface is at least familial and predictable.

It's easy to wrap such a function in an interface that does malloc() if wanted.

Avoid basename() and dirname()

These POSIX functions require a modifiable string, but we can write simpler code that just returns a pointer or index to the start/end of a read-only string.

Multiple strcat() is inefficient

Although this isn't performance-sensitive code, we'd do well to avoid repeated strcat(), where we start at the beginning of the target string in every call - labelled by Joel Spolsky as using Shlemiel the Painter's algorithm.


Modified version

Here's one that addresses the concerns in my review, and only copies the string data once:

#include <stdio.h>
#include <string.h>
#include <time.h>

int make_log_name(char *outbuf, size_t maxlen, time_t timestamp, const char *template)
{
    /* Format the time value */
    struct tm timeval;
    if (!localtime_r(&timestamp, &timeval)) {
        return -1;
    }
    char timebuf[14];           /* match size to next line's format string */
    if (!strftime(timebuf, sizeof timebuf, "-%Y%m%d%H%M", &timeval)) {
        return -1;
    }

    /* Find the insertion point in the template */
    const char* slash = strrchr(template, '/');
    if (!slash) {
        /* not found - use beginning of string */
        slash = template;
    }
    const char* dot = strchr(slash, '.');
    if (!dot) {
        /* not found - use end of basename */
        dot = slash + strlen(slash);
    }

    return snprintf(outbuf, maxlen, "%.*s%s%s",
                    (int)(dot-template), template, timebuf, dot);
}
/* A simple test of the provided input templates */
int main(int argc, char **argv)
{
    const time_t timestamp = time(NULL);
    for (int i = 1;  i < argc;  ++i) {
        char buf[FILENAME_MAX];
        int len = make_log_name(buf, sizeof buf, timestamp, argv[i]);
        if (len < 0 || (size_t)len >= sizeof buf) {
            fprintf(stderr, "Failed for %s\n", argv[i]);
            continue;
        }
        puts(buf);
    }
}

And demonstration of it working with various input values:

./251870 /tmp/mine.log /tmp/log mine.log log .test /a.b/c.d
/tmp/mine-202011111312.log
/tmp/log-202011111312
mine-202011111312.log
log-202011111312
-202011111312.test
/a.b/c-202011111312.d
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.