[systemd-devel] [PATCH] Add some extra __attribute__ ((format)) s

[Zbigniew Jędrzejewski-Szmek]

Hi,


On Tue, Apr 02, 2013 at 04:02:58AM -0300, Cristian Rodríguez wrote:
> 
> diff --git a/src/core/manager.h b/src/core/manager.h
> index 9d8d943..cd1ef23 100644
> --- a/src/core/manager.h
> +++ b/src/core/manager.h
> @@ -301,6 +301,6 @@ void manager_undo_generators(Manager *m);
>  void manager_recheck_journal(Manager *m);
>  
>  void manager_set_show_status(Manager *m, bool b);
> -void manager_status_printf(Manager *m, bool ephemeral, const char *status, const char *format, ...);
> +void manager_status_printf(Manager *m, bool ephemeral, const char *status, const char *format, ...) _printf_attr_(4,5);

unfortunately this one gives a warning during compilation (gcc-4.7.2-8.fc18.x86_64):

../src/core/unit.c: In function 'unit_status_printf':
../src/core/unit.c:2556:9: warning: format not a string literal, argument types not checked [-Wformat-nonliteral]

I tried adding
-void unit_status_printf(Unit *u, const char *status, const char *unit_status_msg_format);
+void unit_status_printf(Unit *u, const char *status, const char *unit_status_msg_format) _printf_attr_(3, 0);

but it doesn't help. Do you know how we can avoid this warning?

>  void watch_init(Watch *w);
> diff --git a/src/journal/journald-server.h b/src/journal/journald-server.h
> index 21edd6b..ffcf2f3 100644
> --- a/src/journal/journald-server.h
> +++ b/src/journal/journald-server.h
> @@ -130,7 +130,7 @@ typedef struct Server {
>  #define N_IOVEC_UDEV_FIELDS 32
>  
>  void server_dispatch_message(Server *s, struct iovec *iovec, unsigned n, unsigned m, struct ucred *ucred, struct timeval *tv, const char *label, size_t label_len, const char *unit_id, int priority);
> -void server_driver_message(Server *s, sd_id128_t message_id, const char *format, ...);
> +void server_driver_message(Server *s, sd_id128_t message_id, const char *format, ...) _printf_attr_(3,4);
>  
>  /* gperf lookup function */
>  const struct ConfigPerfItem* journald_gperf_lookup(const char *key, unsigned length);
> diff --git a/src/journal/microhttpd-util.h b/src/journal/microhttpd-util.h
> index d4fefa7..36c7c10 100644
> --- a/src/journal/microhttpd-util.h
> +++ b/src/journal/microhttpd-util.h
> @@ -23,4 +23,4 @@
>  
>  #include <stdarg.h>
>  
> -void microhttpd_logger(void *arg, const char *fmt, va_list ap);
> +void microhttpd_logger(void *arg, const char *fmt, va_list ap) __attribute__((format(printf, 2, 0)));

Why not use _printf_attr_ here?

> diff --git a/src/shared/log.h b/src/shared/log.h
> index 9aafcb4..5fc8988 100644
> --- a/src/shared/log.h
> +++ b/src/shared/log.h
> @@ -83,7 +83,7 @@ int log_metav(
>                  int line,
>                  const char *func,
>                  const char *format,
> -                va_list ap);
> +                va_list ap) _printf_attr_(5,0);
>  
>  int log_meta_object(
>                  int level,
> @@ -102,14 +102,14 @@ int log_metav_object(
>                  const char *object_name,
>                  const char *object,
>                  const char *format,
> -                va_list ap);
> +                va_list ap) _printf_attr_(7,0);
>  
>  int log_struct_internal(
>                  int level,
>                  const char *file,
>                  int line,
>                  const char *func,
> -                const char *format, ...) _sentinel_;
> +                const char *format, ...) _printf_attr_(5,0) _sentinel_;
>  
>  int log_oom_internal(
>                  const char *file,
> diff --git a/src/shared/util.h b/src/shared/util.h
> index d1cdd90..78d8220 100644
> --- a/src/shared/util.h
> +++ b/src/shared/util.h
> @@ -359,8 +359,8 @@ int pipe_eof(int fd);
>  
>  cpu_set_t* cpu_set_malloc(unsigned *ncpus);
>  
> -int status_vprintf(const char *status, bool ellipse, bool ephemeral, const char *format, va_list ap);
> -int status_printf(const char *status, bool ellipse, bool ephemeral, const char *format, ...);
> +int status_vprintf(const char *status, bool ellipse, bool ephemeral, const char *format, va_list ap) _printf_attr_(4,0);
> +int status_printf(const char *status, bool ellipse, bool ephemeral, const char *format, ...) _printf_attr_(4,5);
>  int status_welcome(void);
>  
>  int fd_columns(int fd);
> diff --git a/src/systemd/sd-bus.h b/src/systemd/sd-bus.h
> index 057931d..6afd79c 100644
> --- a/src/systemd/sd-bus.h
> +++ b/src/systemd/sd-bus.h
> @@ -161,7 +161,7 @@ int sd_bus_get_owner_pid(sd_bus *bus, const char *name, pid_t *pid);
>  #define SD_BUS_ERROR_INIT_CONST(name, message) {(name), (message), 0}
>  
>  void sd_bus_error_free(sd_bus_error *e);
> -int sd_bus_error_set(sd_bus_error *e, const char *name, const char *format, ...);
> +int sd_bus_error_set(sd_bus_error *e, const char *name, const char *format, ...)  __attribute__ ((format (printf, 3, 0)));

I don't think that printf attributes should be added unconditionally
to public functions (this applies to src/system/*.h and src/udev/udev.h).
The problem is that sometimes gcc emits warnings which are hard to get 
rid of. src/systemd/sd-daemon.h does a dance with '#define _sd_printf_attr_',
which at least allows the user of the header to undefine the macro,
avoiding problems. If we add macro to public headers, this pattern
should be followed.

Zbyszek