[virt-tools-list] [virt-viewer PATCH v2 1/3] Annotate message dialog helpers with format(gnu_printf)

[Christophe Fergeau]

The shortlog should mention G_GNUC_PRINTF rather than format(printf)

On Tue, Feb 19, 2019 at 03:00:09PM +0000, Daniel P. Berrangé wrote:
> This allows the compiler to validate the format string and args passed
> to the function.
> 
> Signed-off-by: Daniel P. Berrangé <berrange at redhat.com>
> ---
>  src/virt-viewer-app.c      | 38 +++++++++++++++++++++++---------------
>  src/virt-viewer-notebook.c |  4 ++--
>  2 files changed, 25 insertions(+), 17 deletions(-)
> 
> diff --git a/src/virt-viewer-app.c b/src/virt-viewer-app.c
> index 2592484..15e7b3d 100644
> --- a/src/virt-viewer-app.c
> +++ b/src/virt-viewer-app.c
> @@ -197,19 +197,16 @@ virt_viewer_app_set_debug(gboolean debug)
>      doDebug = debug;
>  }
>  
> -static GtkWidget*
> -virt_viewer_app_make_message_dialog(VirtViewerApp *self,
> -                                    const char *fmt, ...)
> +G_GNUC_PRINTF(2, 0) static GtkWidget*
> +virt_viewer_app_make_message_dialogv(VirtViewerApp *self,
> +                                     const char *fmt, va_list vargs)

Is the addition of this helper required?
I'd put G_GNUC_PRINTF after the declaration:
https://developer.gnome.org/glib/unstable/glib-Miscellaneous-Macros.html#G-GNUC-PRINTF:CAPS
« Place the attribute after the function declaration, just before the
semicolon. »

We don't have a semi-colon here, but at the end of the declaration seems
more consistent with this documentation.


>  {
>      g_return_val_if_fail(VIRT_VIEWER_IS_APP(self), NULL);
>      GtkWindow *window = GTK_WINDOW(virt_viewer_window_get_window(self->priv->main_window));
>      GtkWidget *dialog;
>      char *msg;
> -    va_list vargs;
>  
> -    va_start(vargs, fmt);
>      msg = g_strdup_vprintf(fmt, vargs);
> -    va_end(vargs);
>  
>      dialog = gtk_message_dialog_new(window,
>                                      GTK_DIALOG_MODAL |
> @@ -224,23 +221,34 @@ virt_viewer_app_make_message_dialog(VirtViewerApp *self,
>      return dialog;
>  }
>  
> -void
> +G_GNUC_PRINTF(2, 3) static GtkWidget*
> +virt_viewer_app_make_message_dialog(VirtViewerApp *self,
> +                                    const char *fmt, ...)
> +{
> +    g_return_val_if_fail(VIRT_VIEWER_IS_APP(self), NULL);
> +    GtkWidget *dialog;
> +    va_list vargs;
> +
> +    va_start(vargs, fmt);
> +    dialog = virt_viewer_app_make_message_dialogv(self, fmt, vargs);
> +    va_end(vargs);
> +
> +    return dialog;
> +}
> +
> +G_GNUC_PRINTF(2, 3) void
>  virt_viewer_app_simple_message_dialog(VirtViewerApp *self,
>                                        const char *fmt, ...)

This should be done in the .h file, and same comment about putting this
after the declaration.
Same comments throughout the rest of this file.

Christophe

>  {
>      GtkWidget *dialog;
> -    char *msg;
>      va_list vargs;
>  
>      va_start(vargs, fmt);
> -    msg = g_strdup_vprintf(fmt, vargs);
> +    dialog = virt_viewer_app_make_message_dialogv(self, fmt, vargs);
>      va_end(vargs);
>  
> -    dialog = virt_viewer_app_make_message_dialog(self, msg);
>      gtk_dialog_run(GTK_DIALOG(dialog));
>      gtk_widget_destroy(dialog);
> -
> -    g_free(msg);
>  }
>  
>  static void
> @@ -668,7 +676,7 @@ virt_viewer_app_open_unix_sock(const char *unixsock, GError **error)
>  
>  #endif /* defined(HAVE_SOCKETPAIR) && defined(HAVE_FORK) */
>  
> -void
> +G_GNUC_PRINTF(2, 3) void
>  virt_viewer_app_trace(VirtViewerApp *self,
>                        const char *fmt, ...)
>  {
> @@ -1871,7 +1879,7 @@ virt_viewer_app_on_application_startup(GApplication *app)
>  
>      if (!virt_viewer_app_start(self, &error)) {
>          if (error && !g_error_matches(error, VIRT_VIEWER_ERROR, VIRT_VIEWER_ERROR_CANCELLED))
> -            virt_viewer_app_simple_message_dialog(self, error->message);
> +            virt_viewer_app_simple_message_dialog(self, "%s", error->message);
>  
>          g_clear_error(&error);
>          g_application_quit(app);
> @@ -2477,7 +2485,7 @@ show_status_cb(gpointer value,
>      virt_viewer_notebook_show_status(nb, text);
>  }
>  
> -void
> +G_GNUC_PRINTF(2, 3) void
>  virt_viewer_app_show_status(VirtViewerApp *self, const gchar *fmt, ...)
>  {
>      va_list args;
> diff --git a/src/virt-viewer-notebook.c b/src/virt-viewer-notebook.c
> index 3a74e9f..310ea12 100644
> --- a/src/virt-viewer-notebook.c
> +++ b/src/virt-viewer-notebook.c
> @@ -82,7 +82,7 @@ virt_viewer_notebook_init (VirtViewerNotebook *self)
>      gtk_notebook_append_page(GTK_NOTEBOOK(self), priv->status, NULL);
>  }
>  
> -void
> +G_GNUC_PRINTF(2, 0) void
>  virt_viewer_notebook_show_status_va(VirtViewerNotebook *self, const gchar *fmt, va_list args)
>  {
>      VirtViewerNotebookPrivate *priv;
> @@ -99,7 +99,7 @@ virt_viewer_notebook_show_status_va(VirtViewerNotebook *self, const gchar *fmt,
>      g_free(text);
>  }
>  
> -void
> +G_GNUC_PRINTF(2, 3) void
>  virt_viewer_notebook_show_status(VirtViewerNotebook *self, const gchar *fmt, ...)
>  {
>      va_list args;
> -- 
> 2.20.1
> 
> _______________________________________________
> virt-tools-list mailing list
> virt-tools-list at redhat.com
> https://www.redhat.com/mailman/listinfo/virt-tools-list
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/virt-tools-list/attachments/20190304/370b47fd/attachment.sig>