Repeated use of a variadic function argument doesn't work

Question

I have a function that tries to log stuff to the console and also to a log file, but it doesn't work. The second use of the variable length argument gives garbage written to the console. Any ideas?

    void logPrintf(const char *fmt, ...) {
        va_list ap;    // log to logfile
        va_start(ap, fmt);
        logOpen;
        vfprintf(flog, fmt, ap);
        logClose;
        va_end(ap);
        va_list ap2;   // log to console
        va_start(ap2, fmt);
        printf(fmt, ap2);
        va_end(ap2);
    }

You need to use vprintf the second time, not printf. – Greg Prisament Dec 10 '13 at 19:57 — Greg Prisament, Dec 10 '13 at 19:57

score 15 · Answer 1 · answered Dec 31 '16 at 21:01

The original code fails because it tries to use printf() where it needs to use vprintf(). Taking dubious points like the logOpen and logClose statements at face value (given the notation, presumably they're macros which open and close the flog file stream), the code should be:

void logPrintf(const char *fmt, ...) {
    va_list ap;
    va_start(ap, fmt);
    logOpen;
    vfprintf(flog, fmt, ap);
    logClose;
    va_end(ap);
    va_list ap2;
    va_start(ap2, fmt);
    vprintf(fmt, ap2);
    va_end(ap2);
}

There's no particular requirement to use two separate va_list variables; it is perfectly OK to use the same one twice as long as you use va_end() before you use va_start() again.

void logPrintf(const char *fmt, ...) {
    va_list ap;
    va_start(ap, fmt);
    logOpen;
    vfprintf(flog, fmt, ap);
    logClose;
    va_end(ap);
    va_start(ap, fmt);
    vprintf(fmt, ap);
    va_end(ap);
}

When a va_list value is passed to another function (vfprintf() and vprintf() in this code), you should assume that it is no longer usable in the current function. It is only safe to call va_end() on it.

There is no need for va_copy() in this code. It works, but it isn't needed. You need va_copy() in other circumstances, such as when your function is passed a va_list and you need to process the list twice:

void logVprintf(const char *fmt, va_list args1)
{
    va_list args2;
    va_copy(args2, args1);
    logOpen;
    vfprintf(flog, fmt, args1);
    logClose;
    vprintf(fmt, args2);
    va_end(args2);
}

Note that in this code, it is the calling code's responsibility to call va_end() on args1. Indeed, the standard says:

Each invocation of the va_start and va_copy macros shall be matched by a corresponding invocation of the va_end macro in the same function.

Since the logVprintf() function doesn't call either va_start or va_copy to initialize args1, it cannot legitimately call va_end on args1. On the other hand, the standard requires it to call va_end for args2.

The logPrintf() function can be implemented in terms of logVprintf() now:

void logPrintf(const char *fmt, ...)
{
    va_list args;
    va_start(args, fmt);
    logVprintf(fmt, args);
    va_end(args);
}

This structure — an operational function that takes a va_list and a cover function that takes ellipsis (variable arguments) and passes them to the operational function after conversion to a va_list — is often a good way to work. Sooner or later, you usually find a need for the version with a va_list argument.

This is the correct answer that directly solves OP's question without recommending to change their architecture. — Vortico, Jan 27 '17 at 01:28

Matthieu M. · Answer 2 · 2014-07-21T06:07:35.673

1

Upgrade your compiler, that is more like C++:

template <typename... Args>
void logPrintf(const char *fmt, Args&&... args) {
    logOpen;
    fprintf(flog, fmt, args...);
    logClose;

    printf(fmt, args...);
}

Though of course it would then be good taste to provide typesafe versions of printf and fprintf.

edited Jul 21 '14 at 06:07

answered Feb 16 '12 at 11:17

Matthieu M.

287,565
48
449
722

Above example requires C++11. – David Given Feb 16 '12 at 11:32
1

@DavidGiven: yes (thus the *upgrade your compiler*). – Matthieu M. Feb 16 '12 at 13:08
I think vfprintf needs to be fprintf instead here--you're passing the actual arguments rather than a va_list. – John Zwinck Jul 21 '14 at 01:53

score -2 · Answer 3 · answered Feb 16 '12 at 10:11

-2

I think this way makes more sense:

void logPrintf(const char *fmt, ...) {
        va_list ap;    // log to logfile
        va_start(ap, fmt);
        logOpen;
        vfprintf(flog, fmt, ap); //logfile
         printf(fmt, ap); //console
        logClose;
        va_end(ap);
    }

answered Feb 16 '12 at 10:11

Tony The Lion

61,704
67
242
415

Thanks Tony, but that doesn't work either. It's like the pointer gets left at the end of the list so the second use gets garbage. – Neddie Feb 16 '12 at 10:19
Yup, that's exactly what's happening. `va_list`s are always pass-by-reference. – David Given Feb 16 '12 at 11:33
1

This example (even with `vprintf` instead of `printf`) is wrong according to the man page: "If ap is passed to a function that uses va_arg(ap,type) then the value of ap is undefined after the return of that function." On linux x86-32 it works as intended, but not e.g. on x86-64. – Armin Rigo Dec 09 '15 at 09:38
This code is wrong for the reasons stated by [Armin Rigo](https://stackoverflow.com/users/1556290/armin-rigo) in their [comment](https://stackoverflow.com/questions/9309246/repeated-use-of-a-variadic-function-argument-doesnt-work/9309452#comment56097427_9309341) — and, indeed, both the earlier comments. The solution needs `va_end(ap); va_start(ap, fmt);` inserted between `vfprintf()` and `printf()`, and `printf()` needs to be replaced by `vprintf()`. **Do not try to use this code as written.** – Jonathan Leffler Jan 13 '17 at 22:01

Repeated use of a variadic function argument doesn't work

3 Answers3

Linked