From 5d7a7c18166e0c8088d8ced52f62ff679cded82f Mon Sep 17 00:00:00 2001 From: Brian Neradt Date: Wed, 12 May 2021 23:59:15 +0000 Subject: [PATCH] Apply fmt compile time argument checking to log functions This applies compile time argument checking for the log functions (diag, debug, error, etc.). --- include/ts/apidefs.h.in | 58 ++++++++++++++++++++++++++++++++++-- include/tscore/HTTPVersion.h | 4 +-- include/tscore/LogMessage.h | 24 +++++++-------- include/tscore/ink_apidefs.h | 6 ++-- proxy/http/HttpTransact.cc | 2 +- 5 files changed, 75 insertions(+), 19 deletions(-) diff --git a/include/ts/apidefs.h.in b/include/ts/apidefs.h.in index 5de3277021d..8e03d3a12e1 100644 --- a/include/ts/apidefs.h.in +++ b/include/ts/apidefs.h.in @@ -51,11 +51,65 @@ #define tsapi #endif +/** Apply printf format string compile-time argument checking to a function. + * + * + * For example, given the following function from DiagsTypes.h: + * + * @code + * class Diags { + * + * ... + * + * void + * print( + * const char *tag, + * DiagsLevel level, + * const SourceLocation *loc, + * const char *fmt, + * ...) const; + * + * ... + * + * }; + * @endcode + * + * This macro can be used to apply compiler checking for ... against the fmt + * argument like so: + * + * + * @code + * class Diags { + * + * ... + * + * void + * print( + * const char *tag, + * DiagsLevel level, + * const SourceLocation *loc, + * const char *fmt, + * ...) const TS_PRINTFLIKE(5, 6); + * + * ... + * + * }; + * @endcode + * + * Note in this case, (5, 6) rather than (4, 5) is passed because `this` + * counts as the implicit first parameter of this member function. + * + * @param[in] fmt_index The index of the format string argument, with argument + * indexing starting at 1. + * + * @param[in] arg_index The index of the first format argument string, with + * argument indexing starting at 1. + */ #if !defined(TS_PRINTFLIKE) #if defined(__GNUC__) || defined(__clang__) -#define TS_PRINTFLIKE(fmt, arg) __attribute__((format(printf, fmt, arg))) +#define TS_PRINTFLIKE(fmt_index, arg_index) __attribute__((format(printf, fmt_index, arg_index))) #else -#define TS_PRINTFLIKE(fmt, arg) +#define TS_PRINTFLIKE(fmt_index, arg_index) #endif #endif diff --git a/include/tscore/HTTPVersion.h b/include/tscore/HTTPVersion.h index aafb5e9e70d..b8a15b1df87 100644 --- a/include/tscore/HTTPVersion.h +++ b/include/tscore/HTTPVersion.h @@ -40,7 +40,7 @@ class HTTPVersion uint8_t get_major() const; uint8_t get_minor() const; - int get_flat_version() const; + uint32_t get_flat_version() const; private: uint8_t vmajor = 0; @@ -82,7 +82,7 @@ HTTPVersion::get_minor() const /*------------------------------------------------------------------------- -------------------------------------------------------------------------*/ -inline int +inline uint32_t HTTPVersion::get_flat_version() const { return vmajor << 16 | vminor; diff --git a/include/tscore/LogMessage.h b/include/tscore/LogMessage.h index 69918e6fb32..b8ae22bb5fb 100644 --- a/include/tscore/LogMessage.h +++ b/include/tscore/LogMessage.h @@ -60,18 +60,18 @@ class LogMessage : public Throttler LogMessage(std::chrono::milliseconds throttling_interval); /* TODO: Add BufferWriter overloads for these. */ - void diag(const char *tag, SourceLocation const &loc, const char *fmt, ...); - void debug(const char *tag, SourceLocation const &loc, const char *fmt, ...); - void status(SourceLocation const &loc, const char *fmt, ...); - void note(SourceLocation const &loc, const char *fmt, ...); - void warning(SourceLocation const &loc, const char *fmt, ...); - void error(SourceLocation const &loc, const char *fmt, ...); - void fatal(SourceLocation const &loc, const char *fmt, ...); - void alert(SourceLocation const &loc, const char *fmt, ...); - void emergency(SourceLocation const &loc, const char *fmt, ...); - - void message(DiagsLevel level, SourceLocation const &loc, const char *fmt, ...); - void print(const char *tag, DiagsLevel level, SourceLocation const &loc, const char *fmt, ...); + void diag(const char *tag, SourceLocation const &loc, const char *fmt, ...) TS_PRINTFLIKE(4, 5); + void debug(const char *tag, SourceLocation const &loc, const char *fmt, ...) TS_PRINTFLIKE(4, 5); + void status(SourceLocation const &loc, const char *fmt, ...) TS_PRINTFLIKE(3, 4); + void note(SourceLocation const &loc, const char *fmt, ...) TS_PRINTFLIKE(3, 4); + void warning(SourceLocation const &loc, const char *fmt, ...) TS_PRINTFLIKE(3, 4); + void error(SourceLocation const &loc, const char *fmt, ...) TS_PRINTFLIKE(3, 4); + void fatal(SourceLocation const &loc, const char *fmt, ...) TS_PRINTFLIKE(3, 4); + void alert(SourceLocation const &loc, const char *fmt, ...) TS_PRINTFLIKE(3, 4); + void emergency(SourceLocation const &loc, const char *fmt, ...) TS_PRINTFLIKE(3, 4); + + void message(DiagsLevel level, SourceLocation const &loc, const char *fmt, ...) TS_PRINTFLIKE(4, 5); + void print(const char *tag, DiagsLevel level, SourceLocation const &loc, const char *fmt, ...) TS_PRINTFLIKE(5, 6); void diag_va(const char *tag, SourceLocation const &loc, const char *fmt, va_list args); void debug_va(const char *tag, SourceLocation const &loc, const char *fmt, va_list args); diff --git a/include/tscore/ink_apidefs.h b/include/tscore/ink_apidefs.h index 544a83526d2..5bba7390da1 100644 --- a/include/tscore/ink_apidefs.h +++ b/include/tscore/ink_apidefs.h @@ -54,11 +54,13 @@ /* Enable this to get printf() style warnings on the Inktomi functions. */ /* #define PRINTFLIKE(IDX, FIRST) __attribute__((format (printf, IDX, FIRST))) */ +/** For further information, see the doxygen comments for TS_PRINTFLIKE in + * include/ts/apidefs.h.in */ #if !defined(TS_PRINTFLIKE) #if defined(__GNUC__) || defined(__clang__) -#define TS_PRINTFLIKE(fmt, arg) __attribute__((format(printf, fmt, arg))) +#define TS_PRINTFLIKE(fmt_index, arg_index) __attribute__((format(printf, fmt_index, arg_index))) #else -#define TS_PRINTFLIKE(fmt, arg) +#define TS_PRINTFLIKE(fmt_index, arg_index) #endif #endif diff --git a/proxy/http/HttpTransact.cc b/proxy/http/HttpTransact.cc index d7a14a60c83..4dde9e7cc86 100644 --- a/proxy/http/HttpTransact.cc +++ b/proxy/http/HttpTransact.cc @@ -4031,7 +4031,7 @@ HttpTransact::handle_forward_server_connection_open(State *s) if (real_version != s->host_db_info.app.http_data.http_version) { // Need to update the hostdb s->updated_server_version = real_version; - TxnDebug("http_trans", "Update hostdb history of server HTTP version 0x%x", s->updated_server_version); + TxnDebug("http_trans", "Update hostdb history of server HTTP version 0x%x", s->updated_server_version.get_flat_version()); } s->state_machine->do_hostdb_update_if_necessary();