Re: [PATCH/RFC 02/11] strbuf: add non-variadic function strbuf_vaddf()

Erik Faye-Lund [off-list ref] writes:

On Thu, Nov 26, 2009 at 1:59 AM, Junio C Hamano [off-list ref] wrote:

quoted

Erik Faye-Lund [off-list ref] writes:

quoted

+void strbuf_vaddf(struct strbuf *sb, const char *fmt, va_list ap)
 {
      int len;

      if (!strbuf_avail(sb))
              strbuf_grow(sb, 64);
      len = vsnprintf(sb->buf + sb->len, sb->alloc - sb->len, fmt, ap);
      if (len < 0)
              die("your vsnprintf is broken");
      if (len > strbuf_avail(sb)) {
              strbuf_grow(sb, len);
              len = vsnprintf(sb->buf + sb->len, sb->alloc - sb->len, fmt, ap);
              if (len > strbuf_avail(sb)) {
                      die("this should not happen, your snprintf is broken");
              }

Hmm, I would have expected to see va_copy() somewhere in the patch text.
Is it safe to reuse ap like this in two separate invocations of
vsnprintf()?

I think your expectation is well justified, this seems to be a
portability-bug waiting to happen. Sorry for missing this prior to
sending out - on Windows this is known to work, and this function is
currently only used from the Windows implementation of syslog.

How kosher is it to use va_copy in the git-core, considering that it's
C99? A quick grep reveals only one occurrence of va_copy in the
source, and that's in compat/winansi.c. Searching the history of next
reveals that Alex Riesen (CC'd) already removed one occurrence
(4bf5383), so I'm starting to get slightly scared it might not be OK.

We tend to avoid C99 features and it saved us in a few occasions.  Recent
MSVC port revealed that we still had a handful of decl-after-statments but
luckily the necessary fix-ups were minimal because I have been reasonably
careful to reject patches that add it long before MSVC port happened.

In practice it seems that something like the following works
portably-enough for many applications, dunno if it's something we'll
be happy with:
#ifndef va_copy
#define va_copy(a,b) ((a) = (b))
#endif

Since an obvious implementation of va_list would be to make it a pointer
into the stack frame, doing the above would work on many systems.  On
esoteric systems that needs something different (e.g. where va_list is
implemented as a size-1 array of pointers, va_copy(a,b) needs to be an
assignment (*(a) = *(b))), people can add compatibility macro later.

Historically some systems that do have a suitable implementation had it
under the name __va_copy() instead, so it would have been better to define
it as something like:

    #ifndef va_copy
    # ifdef __va_copy
    # define va_copy(a,b) __va_copy(a,b)
    # else
    # /* fallback for the most obvious implementation of va_list */
    # define va_copy(a,b) ((a) = (b))
    # endif
    #endif

But I do not know it still matters in practice anymore.

On Thu, Nov 26, 2009 at 7:46 PM, Junio C Hamano [off-list ref] wrote:

Erik Faye-Lund [off-list ref] writes:

quoted

In practice it seems that something like the following works
portably-enough for many applications, dunno if it's something we'll
be happy with:
#ifndef va_copy
#define va_copy(a,b) ((a) = (b))
#endif

Since an obvious implementation of va_list would be to make it a pointer
into the stack frame, doing the above would work on many systems.  On
esoteric systems that needs something different (e.g. where va_list is
implemented as a size-1 array of pointers, va_copy(a,b) needs to be an
assignment (*(a) = *(b))), people can add compatibility macro later.

Historically some systems that do have a suitable implementation had it
under the name __va_copy() instead, so it would have been better to define
it as something like:

   #ifndef va_copy
   # ifdef __va_copy
   # define va_copy(a,b) __va_copy(a,b)
   # else
   # /* fallback for the most obvious implementation of va_list */
   # define va_copy(a,b) ((a) = (b))
   # endif
   #endif

But I do not know it still matters in practice anymore.

Perhaps I can do one better: use memcpy instead of standard
assignment. The Autoconf manual[1] suggests that it's more portable.
Something like this:

#ifndef va_copy
# ifdef __va_copy
#  define va_copy(a,b) __va_copy(a,b)
# else
#  define va_copy(a,b) memcpy(&a, &b, sizeof (va_list))
# endif
#endif

I'll add this to git-compat-util.h this for the next round unless
someone yells really loud at me.

*[1] http://www.gnu.org/software/hello/manual/autoconf/Function-Portability.html#index-g_t_0040code_007bva_005fcopy_007d-357
-- 
Erik "kusma" Faye-Lund

Erik Faye-Lund schrieb:

Perhaps I can do one better: use memcpy instead of standard
assignment. The Autoconf manual[1] suggests that it's more portable.
Something like this:

#ifndef va_copy
# ifdef __va_copy
#  define va_copy(a,b) __va_copy(a,b)
# else
#  define va_copy(a,b) memcpy(&a, &b, sizeof (va_list))
# endif
#endif

I'll add this to git-compat-util.h this for the next round unless
someone yells really loud at me.

As I said elsewhere in the thread, I do not see enough reason to add
strbuf_vaddf() only for a syslog() emulation in the first place.

-- Hannes