| Message ID | 20230705141205.525776-2-kkostiuk@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | QGA VSS Logging | expand |
Hi Konstantin, On 5/7/23 16:12, Konstantin Kostiuk wrote: > Signed-off-by: Konstantin Kostiuk <kkostiuk@redhat.com> > --- > qga/vss-win32/vss-debug.h | 31 +++++++++++++++++++++++++++++++ > 1 file changed, 31 insertions(+) > create mode 100644 qga/vss-win32/vss-debug.h > +#define PRINT_DEBUG(fmt, ...) { \ > + char user_sting[512] = { 0 }; \ > + char full_string[640] = { 0 }; \ > + snprintf(user_sting, 512, fmt, ## __VA_ARGS__); \ > + snprintf(full_string, 640, QGA_PROVIDER_NAME"[%lu]: %s %s\n", \ > + GetCurrentThreadId(), __func__, user_sting); \ > + OutputDebugString(full_string); \ > + fprintf(stderr, "%s", full_string); \ > +} Why not simply use a plain function? > +#define PRINT_DEBUG_BEGIN PRINT_DEBUG("begin") > +#define PRINT_DEBUG_END PRINT_DEBUG("end") > + > +#endif
Hi Philippe, On Wed, Jul 5, 2023 at 11:35 PM Philippe Mathieu-Daudé <philmd@linaro.org> wrote: > Hi Konstantin, > > On 5/7/23 16:12, Konstantin Kostiuk wrote: > > Signed-off-by: Konstantin Kostiuk <kkostiuk@redhat.com> > > --- > > qga/vss-win32/vss-debug.h | 31 +++++++++++++++++++++++++++++++ > > 1 file changed, 31 insertions(+) > > create mode 100644 qga/vss-win32/vss-debug.h > > > > +#define PRINT_DEBUG(fmt, ...) { > \ > > + char user_sting[512] = { 0 }; > \ > > + char full_string[640] = { 0 }; > \ > > + snprintf(user_sting, 512, fmt, ## __VA_ARGS__); > \ > > + snprintf(full_string, 640, QGA_PROVIDER_NAME"[%lu]: %s %s\n", > \ > > + GetCurrentThreadId(), __func__, user_sting); > \ > > + OutputDebugString(full_string); > \ > > + fprintf(stderr, "%s", full_string); > \ > > +} > > Why not simply use a plain function? > I am not sure what you mean. If you mean to call OutputDebugString directly, then we need to concatenate the proper string for each call. If you mean convert PRINT_DEBUG to function, then we can't use the __func__ macro in PRINT_DEBUG to get the real function name. We can convert PRINT_DEBUG to function and a new macro that will call PRINT_DEBUG and pass a proper value of __fucn__. What solution is better there? > > > +#define PRINT_DEBUG_BEGIN PRINT_DEBUG("begin") > > +#define PRINT_DEBUG_END PRINT_DEBUG("end") > > + > > +#endif > >
On 6/7/23 09:54, Konstantin Kostiuk wrote: > Hi Philippe, > > On Wed, Jul 5, 2023 at 11:35 PM Philippe Mathieu-Daudé > <philmd@linaro.org <mailto:philmd@linaro.org>> wrote: > > Hi Konstantin, > > On 5/7/23 16:12, Konstantin Kostiuk wrote: > > Signed-off-by: Konstantin Kostiuk <kkostiuk@redhat.com > <mailto:kkostiuk@redhat.com>> > > --- > > qga/vss-win32/vss-debug.h | 31 +++++++++++++++++++++++++++++++ > > 1 file changed, 31 insertions(+) > > create mode 100644 qga/vss-win32/vss-debug.h > > > > +#define PRINT_DEBUG(fmt, ...) { > \ > > + char user_sting[512] = { 0 }; > \ > > + char full_string[640] = { 0 }; > \ > > + snprintf(user_sting, 512, fmt, ## __VA_ARGS__); > \ > > + snprintf(full_string, 640, QGA_PROVIDER_NAME"[%lu]: %s > %s\n", \ > > + GetCurrentThreadId(), __func__, user_sting); > \ > > + OutputDebugString(full_string); > \ > > + fprintf(stderr, "%s", full_string); > \ > > +} > > Why not simply use a plain function? > > > I am not sure what you mean. > > If you mean to call OutputDebugString directly, then we need to > concatenate the proper string > for each call. > If you mean convert PRINT_DEBUG to function, then we can't use the > __func__ macro in > PRINT_DEBUG to get the real function name. We can convert PRINT_DEBUG to > function > and a new macro that will call PRINT_DEBUG and pass a proper value of > __fucn__. > > What solution is better there? void qga_debug(const char *funcname, const char *fmt, ...); and g_strdup_vprintf()?
On Thu, Jul 6, 2023 at 1:01 PM Philippe Mathieu-Daudé <philmd@linaro.org> wrote: > On 6/7/23 09:54, Konstantin Kostiuk wrote: > > Hi Philippe, > > > > On Wed, Jul 5, 2023 at 11:35 PM Philippe Mathieu-Daudé > > <philmd@linaro.org <mailto:philmd@linaro.org>> wrote: > > > > Hi Konstantin, > > > > On 5/7/23 16:12, Konstantin Kostiuk wrote: > > > Signed-off-by: Konstantin Kostiuk <kkostiuk@redhat.com > > <mailto:kkostiuk@redhat.com>> > > > --- > > > qga/vss-win32/vss-debug.h | 31 +++++++++++++++++++++++++++++++ > > > 1 file changed, 31 insertions(+) > > > create mode 100644 qga/vss-win32/vss-debug.h > > > > > > > +#define PRINT_DEBUG(fmt, ...) { > > \ > > > + char user_sting[512] = { 0 }; > > \ > > > + char full_string[640] = { 0 }; > > \ > > > + snprintf(user_sting, 512, fmt, ## __VA_ARGS__); > > \ > > > + snprintf(full_string, 640, QGA_PROVIDER_NAME"[%lu]: %s > > %s\n", \ > > > + GetCurrentThreadId(), __func__, user_sting); > > \ > > > + OutputDebugString(full_string); > > \ > > > + fprintf(stderr, "%s", full_string); > > \ > > > +} > > > > Why not simply use a plain function? > > > > > > I am not sure what you mean. > > > > If you mean to call OutputDebugString directly, then we need to > > concatenate the proper string > > for each call. > > If you mean convert PRINT_DEBUG to function, then we can't use the > > __func__ macro in > > PRINT_DEBUG to get the real function name. We can convert PRINT_DEBUG to > > function > > and a new macro that will call PRINT_DEBUG and pass a proper value of > > __fucn__. > > > > What solution is better there? > > void qga_debug(const char *funcname, const char *fmt, ...); > > and g_strdup_vprintf()? > I agree about the qga_debug function. About g_strdup_vprintf I am not sure. Currently, VSS uses only one function from GLib (g_assertion_message_expr). In the past, we had problems with GLib due to mistakes in Windows implementation and we thinking about removing GLib from VSS at all. As VSS.DLL is also invoked directly from Windows, we should use standard C++ types in public functions. We can use GLib types/functions for the internal part of DLL, but I think it will be confusing.
On 6/7/23 12:58, Konstantin Kostiuk wrote: > > > On Thu, Jul 6, 2023 at 1:01 PM Philippe Mathieu-Daudé <philmd@linaro.org > <mailto:philmd@linaro.org>> wrote: > > On 6/7/23 09:54, Konstantin Kostiuk wrote: > > Hi Philippe, > > > > On Wed, Jul 5, 2023 at 11:35 PM Philippe Mathieu-Daudé > > <philmd@linaro.org <mailto:philmd@linaro.org> > <mailto:philmd@linaro.org <mailto:philmd@linaro.org>>> wrote: > > > > Hi Konstantin, > > > > On 5/7/23 16:12, Konstantin Kostiuk wrote: > > > Signed-off-by: Konstantin Kostiuk <kkostiuk@redhat.com > <mailto:kkostiuk@redhat.com> > > <mailto:kkostiuk@redhat.com <mailto:kkostiuk@redhat.com>>> > > > --- > > > qga/vss-win32/vss-debug.h | 31 > +++++++++++++++++++++++++++++++ > > > 1 file changed, 31 insertions(+) > > > create mode 100644 qga/vss-win32/vss-debug.h > > > > > > > +#define PRINT_DEBUG(fmt, ...) { > > \ > > > + char user_sting[512] = { 0 }; > > \ > > > + char full_string[640] = { 0 }; > > \ > > > + snprintf(user_sting, 512, fmt, ## __VA_ARGS__); > > \ > > > + snprintf(full_string, 640, QGA_PROVIDER_NAME"[%lu]: %s > > %s\n", \ > > > + GetCurrentThreadId(), __func__, user_sting); > > \ > > > + OutputDebugString(full_string); > > \ > > > + fprintf(stderr, "%s", full_string); > > \ > > > +} > > > > Why not simply use a plain function? > > > > > > I am not sure what you mean. > > > > If you mean to call OutputDebugString directly, then we need to > > concatenate the proper string > > for each call. > > If you mean convert PRINT_DEBUG to function, then we can't use the > > __func__ macro in > > PRINT_DEBUG to get the real function name. We can convert > PRINT_DEBUG to > > function > > and a new macro that will call PRINT_DEBUG and pass a proper > value of > > __fucn__. > > > > What solution is better there? > > void qga_debug(const char *funcname, const char *fmt, ...); > > and g_strdup_vprintf()? > > > > I agree about the qga_debug function. Alternatively: #define qga_debug(fmt, args...) qga_debug_internal(__func__, fmt, args) void qga_debug_internal(const char *funcname, const char *fmt, ...); > About g_strdup_vprintf I am not sure. Currently, VSS uses only one > function from GLib (g_assertion_message_expr). > In the past, we had problems with GLib due to mistakes in Windows > implementation and we thinking about > removing GLib from VSS at all. As VSS.DLL is also invoked directly from > Windows, we should use standard > C++ types in public functions. We can use GLib types/functions for the > internal part of DLL, but I think it will > be confusing. OK, then vfprintf/vsnprintf :)
diff --git a/qga/vss-win32/vss-debug.h b/qga/vss-win32/vss-debug.h new file mode 100644 index 0000000000..c0bdf7a3fc --- /dev/null +++ b/qga/vss-win32/vss-debug.h @@ -0,0 +1,31 @@ +/* + * QEMU Guest Agent VSS debug declarations + * + * Copyright (C) 2023 Red Hat Inc + * + * Authors: + * Konstantin Kostiuk <kkostiuk@redhat.com> + * + * This work is licensed under the terms of the GNU GPL, version 2 or later. + * See the COPYING file in the top-level directory. + */ + +#include <vss-handles.h> + +#ifndef VSS_DEBUG_H +#define VSS_DEBUG_H + +#define PRINT_DEBUG(fmt, ...) { \ + char user_sting[512] = { 0 }; \ + char full_string[640] = { 0 }; \ + snprintf(user_sting, 512, fmt, ## __VA_ARGS__); \ + snprintf(full_string, 640, QGA_PROVIDER_NAME"[%lu]: %s %s\n", \ + GetCurrentThreadId(), __func__, user_sting); \ + OutputDebugString(full_string); \ + fprintf(stderr, "%s", full_string); \ +} + +#define PRINT_DEBUG_BEGIN PRINT_DEBUG("begin") +#define PRINT_DEBUG_END PRINT_DEBUG("end") + +#endif
Signed-off-by: Konstantin Kostiuk <kkostiuk@redhat.com> --- qga/vss-win32/vss-debug.h | 31 +++++++++++++++++++++++++++++++ 1 file changed, 31 insertions(+) create mode 100644 qga/vss-win32/vss-debug.h