[PATCH 0/4] QGA VSS Logging

[PATCH 0/4] QGA VSS Logging

[Konstantin Kostiuk]

Print all VSS error and trace to debugger and stderr.

Konstantin Kostiuk (4):
  QGA VSS: Add wrapper to send log to debugger and stderr
  QGA VSS: Replace 'fprintf(stderr' with PRINT_DEBUG
  QGA VSS: Print error in err_set
  QGA VSS: Add log in functions begin/end

 qga/vss-win32/install.cpp   | 46 ++++++++++++++++++++++++++++-----
 qga/vss-win32/provider.cpp  |  3 +++
 qga/vss-win32/requester.cpp | 51 ++++++++++++++++++++++++++++++++-----
 qga/vss-win32/vss-debug.h   | 31 ++++++++++++++++++++++
 4 files changed, 118 insertions(+), 13 deletions(-)
 create mode 100644 qga/vss-win32/vss-debug.h

--
2.34.1

[PATCH 1/4] QGA VSS: Add wrapper to send log to debugger and stderr

[Konstantin Kostiuk]

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

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
-- 
2.34.1

[PATCH 2/4] QGA VSS: Replace 'fprintf(stderr' with PRINT_DEBUG

[Konstantin Kostiuk]

Signed-off-by: Konstantin Kostiuk <kkostiuk@redhat.com>
---
 qga/vss-win32/install.cpp   | 13 +++++++------
 qga/vss-win32/requester.cpp |  9 +++++----
 2 files changed, 12 insertions(+), 10 deletions(-)

diff --git a/qga/vss-win32/install.cpp b/qga/vss-win32/install.cpp
index ff93b08a9e..c10a397e51 100644
--- a/qga/vss-win32/install.cpp
+++ b/qga/vss-win32/install.cpp
@@ -13,6 +13,7 @@
 #include "qemu/osdep.h"
 
 #include "vss-common.h"
+#include "vss-debug.h"
 #ifdef HAVE_VSS_SDK
 #include <vscoordint.h>
 #else
@@ -54,7 +55,7 @@ void errmsg(DWORD err, const char *text)
                   FORMAT_MESSAGE_FROM_SYSTEM | FORMAT_MESSAGE_IGNORE_INSERTS,
                   NULL, err, MAKELANGID(LANG_NEUTRAL, SUBLANG_DEFAULT),
                   (char *)&msg, 0, NULL);
-    fprintf(stderr, "%.*s. (Error: %lx) %s\n", len, text, err, msg);
+    PRINT_DEBUG("%.*s. (Error: %lx) %s\n", len, text, err, msg);
     LocalFree(msg);
 }
 
@@ -219,7 +220,7 @@ static HRESULT QGAProviderRemove(ICatalogCollection *coll, int i, void *arg)
 {
     HRESULT hr;
 
-    fprintf(stderr, "Removing COM+ Application: %s\n", QGA_PROVIDER_NAME);
+    PRINT_DEBUG("Removing COM+ Application: %s", QGA_PROVIDER_NAME);
     chk(coll->Remove(i));
 out:
     return hr;
@@ -304,9 +305,9 @@ STDAPI COMRegister(void)
     }
     strcpy(tlbPath, dllPath);
     strcpy(tlbPath+n-3, "tlb");
-    fprintf(stderr, "Registering " QGA_PROVIDER_NAME ":\n");
-    fprintf(stderr, "  %s\n", dllPath);
-    fprintf(stderr, "  %s\n", tlbPath);
+    PRINT_DEBUG("Registering " QGA_PROVIDER_NAME ":");
+    PRINT_DEBUG("  %s", dllPath);
+    PRINT_DEBUG("  %s", tlbPath);
     if (!PathFileExists(tlbPath)) {
         hr = HRESULT_FROM_WIN32(ERROR_FILE_NOT_FOUND);
         errmsg(hr, "Failed to lookup tlb");
@@ -517,7 +518,7 @@ namespace _com_util
         }
 
         if (mbstowcs(bstr, ascii, len) == (size_t)-1) {
-            fprintf(stderr, "Failed to convert string '%s' into BSTR", ascii);
+            PRINT_DEBUG("Failed to convert string '%s' into BSTR", ascii);
             bstr[0] = 0;
         }
         return bstr;
diff --git a/qga/vss-win32/requester.cpp b/qga/vss-win32/requester.cpp
index 3e998af4a8..e4f7013c62 100644
--- a/qga/vss-win32/requester.cpp
+++ b/qga/vss-win32/requester.cpp
@@ -12,6 +12,7 @@
 
 #include "qemu/osdep.h"
 #include "vss-common.h"
+#include "vss-debug.h"
 #include "requester.h"
 #include "install.h"
 #include <vswriter.h>
@@ -59,13 +60,13 @@ STDAPI requester_init(void)
         NULL, -1, NULL, NULL, RPC_C_AUTHN_LEVEL_PKT_PRIVACY,
         RPC_C_IMP_LEVEL_IDENTIFY, NULL, EOAC_NONE, NULL);
     if (FAILED(hr)) {
-        fprintf(stderr, "failed to CoInitializeSecurity (error %lx)\n", hr);
+        PRINT_DEBUG("failed to CoInitializeSecurity (error %lx)", hr);
         return hr;
     }
 
     hLib = LoadLibraryA("VSSAPI.DLL");
     if (!hLib) {
-        fprintf(stderr, "failed to load VSSAPI.DLL\n");
+        PRINT_DEBUG("failed to load VSSAPI.DLL");
         return HRESULT_FROM_WIN32(GetLastError());
     }
 
@@ -78,14 +79,14 @@ STDAPI requester_init(void)
 #endif
         );
     if (!pCreateVssBackupComponents) {
-        fprintf(stderr, "failed to get proc address from VSSAPI.DLL\n");
+        PRINT_DEBUG("failed to get proc address from VSSAPI.DLL");
         return HRESULT_FROM_WIN32(GetLastError());
     }
 
     pVssFreeSnapshotProperties = (t_VssFreeSnapshotProperties)
         GetProcAddress(hLib, "VssFreeSnapshotProperties");
     if (!pVssFreeSnapshotProperties) {
-        fprintf(stderr, "failed to get proc address from VSSAPI.DLL\n");
+        PRINT_DEBUG("failed to get proc address from VSSAPI.DLL");
         return HRESULT_FROM_WIN32(GetLastError());
     }
 
-- 
2.34.1

[PATCH 3/4] QGA VSS: Print error in err_set

[Konstantin Kostiuk]

Signed-off-by: Konstantin Kostiuk <kkostiuk@redhat.com>
---
 qga/vss-win32/requester.cpp | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/qga/vss-win32/requester.cpp b/qga/vss-win32/requester.cpp
index e4f7013c62..73c32381eb 100644
--- a/qga/vss-win32/requester.cpp
+++ b/qga/vss-win32/requester.cpp
@@ -26,9 +26,11 @@
 
 #define DEFAULT_VSS_BACKUP_TYPE VSS_BT_FULL
 
-#define err_set(e, err, fmt, ...)                                           \
-    ((e)->error_setg_win32_wrapper((e)->errp, __FILE__, __LINE__, __func__, \
-                                   err, fmt, ## __VA_ARGS__))
+#define err_set(e, err, fmt, ...) {                                         \
+    (e)->error_setg_win32_wrapper((e)->errp, __FILE__, __LINE__, __func__,  \
+                                   err, fmt, ## __VA_ARGS__);               \
+    PRINT_DEBUG(fmt, ## __VA_ARGS__);                                       \
+}
 /* Bad idea, works only when (e)->errp != NULL: */
 #define err_is_set(e) ((e)->errp && *(e)->errp)
 /* To lift this restriction, error_propagate(), like we do in QEMU code */
-- 
2.34.1

[PATCH 4/4] QGA VSS: Add log in functions begin/end

[Konstantin Kostiuk]

Signed-off-by: Konstantin Kostiuk <kkostiuk@redhat.com>
---
 qga/vss-win32/install.cpp   | 33 +++++++++++++++++++++++++++++++++
 qga/vss-win32/provider.cpp  |  3 +++
 qga/vss-win32/requester.cpp | 34 ++++++++++++++++++++++++++++++++++
 3 files changed, 70 insertions(+)

diff --git a/qga/vss-win32/install.cpp b/qga/vss-win32/install.cpp
index c10a397e51..8364f60a30 100644
--- a/qga/vss-win32/install.cpp
+++ b/qga/vss-win32/install.cpp
@@ -100,6 +100,8 @@ HRESULT put_Value(ICatalogObject *pObj, LPCWSTR name, T val)
 /* Lookup Administrators group name from winmgmt */
 static HRESULT GetAdminName(_bstr_t *name)
 {
+    PRINT_DEBUG_BEGIN;
+
     HRESULT hr;
     COMPointer<IWbemLocator> pLoc;
     COMPointer<IWbemServices> pSvc;
@@ -142,6 +144,7 @@ static HRESULT GetAdminName(_bstr_t *name)
     }
 
 out:
+    PRINT_DEBUG_END;
     return hr;
 }
 
@@ -149,6 +152,8 @@ out:
 static HRESULT getNameByStringSID(
     const wchar_t *sid, LPWSTR buffer, LPDWORD bufferLen)
 {
+    PRINT_DEBUG_BEGIN;
+
     HRESULT hr = S_OK;
     PSID psid = NULL;
     SID_NAME_USE groupType;
@@ -168,6 +173,7 @@ static HRESULT getNameByStringSID(
     LocalFree(psid);
 
 out:
+    PRINT_DEBUG_END;
     return hr;
 }
 
@@ -175,6 +181,8 @@ out:
 static HRESULT QGAProviderFind(
     HRESULT (*found)(ICatalogCollection *, int, void *), void *arg)
 {
+    PRINT_DEBUG_BEGIN;
+
     HRESULT hr;
     COMInitializer initializer;
     COMPointer<IUnknown> pUnknown;
@@ -205,12 +213,15 @@ static HRESULT QGAProviderFind(
     chk(pColl->SaveChanges(&n));
 
 out:
+    PRINT_DEBUG_END;
     return hr;
 }
 
 /* Count QGA VSS provider in COM+ Application Catalog */
 static HRESULT QGAProviderCount(ICatalogCollection *coll, int i, void *arg)
 {
+    PRINT_DEBUG_BEGIN;
+
     (*(int *)arg)++;
     return S_OK;
 }
@@ -218,28 +229,35 @@ static HRESULT QGAProviderCount(ICatalogCollection *coll, int i, void *arg)
 /* Remove QGA VSS provider from COM+ Application Catalog Collection */
 static HRESULT QGAProviderRemove(ICatalogCollection *coll, int i, void *arg)
 {
+    PRINT_DEBUG_BEGIN;
     HRESULT hr;
 
     PRINT_DEBUG("Removing COM+ Application: %s", QGA_PROVIDER_NAME);
     chk(coll->Remove(i));
 out:
+    PRINT_DEBUG_END;
     return hr;
 }
 
 /* Unregister this module from COM+ Applications Catalog */
 STDAPI COMUnregister(void)
 {
+    PRINT_DEBUG_BEGIN;
+
     HRESULT hr;
 
     DllUnregisterServer();
     chk(QGAProviderFind(QGAProviderRemove, NULL));
 out:
+    PRINT_DEBUG_END;
     return hr;
 }
 
 /* Register this module to COM+ Applications Catalog */
 STDAPI COMRegister(void)
 {
+    PRINT_DEBUG_BEGIN;
+
     HRESULT hr;
     COMInitializer initializer;
     COMPointer<IUnknown> pUnknown;
@@ -259,12 +277,14 @@ STDAPI COMRegister(void)
 
     if (!g_hinstDll) {
         errmsg(E_FAIL, "Failed to initialize DLL");
+        PRINT_DEBUG_END;
         return E_FAIL;
     }
 
     chk(QGAProviderFind(QGAProviderCount, (void *)&count));
     if (count) {
         errmsg(E_ABORT, "QGA VSS Provider is already installed");
+        PRINT_DEBUG_END;
         return E_ABORT;
     }
 
@@ -355,6 +375,7 @@ out:
         COMUnregister();
     }
 
+    PRINT_DEBUG_END;
     return hr;
 }
 
@@ -370,6 +391,8 @@ STDAPI_(void) CALLBACK DLLCOMUnregister(HWND, HINSTANCE, LPSTR, int)
 
 static BOOL CreateRegistryKey(LPCTSTR key, LPCTSTR value, LPCTSTR data)
 {
+    PRINT_DEBUG_BEGIN;
+
     HKEY  hKey;
     LONG  ret;
     DWORD size;
@@ -390,6 +413,7 @@ static BOOL CreateRegistryKey(LPCTSTR key, LPCTSTR value, LPCTSTR data)
     RegCloseKey(hKey);
 
 out:
+    PRINT_DEBUG_END;
     if (ret != ERROR_SUCCESS) {
         /* As we cannot printf within DllRegisterServer(), show a dialog. */
         errmsg_dialog(ret, "Cannot add registry", key);
@@ -401,6 +425,8 @@ out:
 /* Register this dll as a VSS provider */
 STDAPI DllRegisterServer(void)
 {
+    PRINT_DEBUG_BEGIN;
+
     COMInitializer initializer;
     COMPointer<IVssAdmin> pVssAdmin;
     HRESULT hr = E_FAIL;
@@ -479,12 +505,15 @@ out:
         DllUnregisterServer();
     }
 
+    PRINT_DEBUG_END;
     return hr;
 }
 
 /* Unregister this VSS hardware provider from the system */
 STDAPI DllUnregisterServer(void)
 {
+    PRINT_DEBUG_BEGIN;
+
     TCHAR key[256];
     COMInitializer initializer;
     COMPointer<IVssAdmin> pVssAdmin;
@@ -502,6 +531,7 @@ STDAPI DllUnregisterServer(void)
     SHDeleteKey(HKEY_CLASSES_ROOT, key);
     SHDeleteKey(HKEY_CLASSES_ROOT, g_szProgid);
 
+    PRINT_DEBUG_END;
     return S_OK; /* Uninstall should never fail */
 }
 
@@ -528,6 +558,8 @@ namespace _com_util
 /* Stop QGA VSS provider service using Winsvc API  */
 STDAPI StopService(void)
 {
+    PRINT_DEBUG_BEGIN;
+
     HRESULT hr = S_OK;
     SC_HANDLE manager = OpenSCManager(NULL, NULL, SC_MANAGER_ALL_ACCESS);
     SC_HANDLE service = NULL;
@@ -552,5 +584,6 @@ STDAPI StopService(void)
 out:
     CloseServiceHandle(service);
     CloseServiceHandle(manager);
+    PRINT_DEBUG_END;
     return hr;
 }
diff --git a/qga/vss-win32/provider.cpp b/qga/vss-win32/provider.cpp
index 1b885e24ee..4405fb00f1 100644
--- a/qga/vss-win32/provider.cpp
+++ b/qga/vss-win32/provider.cpp
@@ -12,6 +12,7 @@
 
 #include "qemu/osdep.h"
 #include "vss-common.h"
+#include "vss-debug.h"
 #ifdef HAVE_VSS_SDK
 #include <vscoordint.h>
 #else
@@ -529,9 +530,11 @@ STDAPI DllCanUnloadNow()
 EXTERN_C
 BOOL WINAPI DllMain(HINSTANCE hinstDll, DWORD dwReason, LPVOID lpReserved)
 {
+    PRINT_DEBUG("begin, reason = %lu", dwReason);
     if (dwReason == DLL_PROCESS_ATTACH) {
         g_hinstDll = hinstDll;
         DisableThreadLibraryCalls(hinstDll);
     }
+    PRINT_DEBUG_END;
     return TRUE;
 }
diff --git a/qga/vss-win32/requester.cpp b/qga/vss-win32/requester.cpp
index 73c32381eb..38c15e4d87 100644
--- a/qga/vss-win32/requester.cpp
+++ b/qga/vss-win32/requester.cpp
@@ -57,6 +57,8 @@ static struct QGAVSSContext {
 
 STDAPI requester_init(void)
 {
+    PRINT_DEBUG_BEGIN;
+
     COMInitializer initializer; /* to call CoInitializeSecurity */
     HRESULT hr = CoInitializeSecurity(
         NULL, -1, NULL, NULL, RPC_C_AUTHN_LEVEL_PKT_PRIVACY,
@@ -92,11 +94,14 @@ STDAPI requester_init(void)
         return HRESULT_FROM_WIN32(GetLastError());
     }
 
+    PRINT_DEBUG_END;
     return S_OK;
 }
 
 static void requester_cleanup(void)
 {
+    PRINT_DEBUG_BEGIN;
+
     if (vss_ctx.hEventFrozen) {
         CloseHandle(vss_ctx.hEventFrozen);
         vss_ctx.hEventFrozen = NULL;
@@ -118,10 +123,13 @@ static void requester_cleanup(void)
         vss_ctx.pVssbc = NULL;
     }
     vss_ctx.cFrozenVols = 0;
+    PRINT_DEBUG_END;
 }
 
 STDAPI requester_deinit(void)
 {
+    PRINT_DEBUG_BEGIN;
+
     requester_cleanup();
 
     pCreateVssBackupComponents = NULL;
@@ -131,11 +139,14 @@ STDAPI requester_deinit(void)
         hLib = NULL;
     }
 
+    PRINT_DEBUG_END;
     return S_OK;
 }
 
 static HRESULT WaitForAsync(IVssAsync *pAsync)
 {
+    PRINT_DEBUG_BEGIN;
+
     HRESULT ret, hr;
 
     do {
@@ -151,11 +162,14 @@ static HRESULT WaitForAsync(IVssAsync *pAsync)
         }
     } while (ret == VSS_S_ASYNC_PENDING);
 
+    PRINT_DEBUG_END;
     return ret;
 }
 
 static void AddComponents(ErrorSet *errset)
 {
+    PRINT_DEBUG_BEGIN;
+
     unsigned int cWriters, i;
     VSS_ID id, idInstance, idWriter;
     BSTR bstrWriterName = NULL;
@@ -237,17 +251,21 @@ out:
     if (pComponent && info) {
         pComponent->FreeComponentInfo(info);
     }
+    PRINT_DEBUG_END;
 }
 
 DWORD get_reg_dword_value(HKEY baseKey, LPCSTR subKey, LPCSTR valueName,
                           DWORD defaultData)
 {
+    PRINT_DEBUG_BEGIN;
+
     DWORD regGetValueError;
     DWORD dwordData;
     DWORD dataSize = sizeof(DWORD);
 
     regGetValueError = RegGetValue(baseKey, subKey, valueName, RRF_RT_DWORD,
                                    NULL, &dwordData, &dataSize);
+    PRINT_DEBUG_END;
     if (regGetValueError  != ERROR_SUCCESS) {
         return defaultData;
     }
@@ -262,6 +280,8 @@ bool is_valid_vss_backup_type(VSS_BACKUP_TYPE vssBT)
 VSS_BACKUP_TYPE get_vss_backup_type(
     VSS_BACKUP_TYPE defaultVssBT = DEFAULT_VSS_BACKUP_TYPE)
 {
+    PRINT_DEBUG_BEGIN;
+
     VSS_BACKUP_TYPE vssBackupType;
 
     vssBackupType = static_cast<VSS_BACKUP_TYPE>(
@@ -269,6 +289,7 @@ VSS_BACKUP_TYPE get_vss_backup_type(
                                                 QGA_PROVIDER_REGISTRY_ADDRESS,
                                                 "VssOption",
                                                 defaultVssBT));
+    PRINT_DEBUG_END;
     if (!is_valid_vss_backup_type(vssBackupType)) {
         return defaultVssBT;
     }
@@ -277,6 +298,8 @@ VSS_BACKUP_TYPE get_vss_backup_type(
 
 void requester_freeze(int *num_vols, void *mountpoints, ErrorSet *errset)
 {
+    PRINT_DEBUG_BEGIN;
+
     COMPointer<IVssAsync> pAsync;
     HANDLE volume;
     HRESULT hr;
@@ -292,6 +315,7 @@ void requester_freeze(int *num_vols, void *mountpoints, ErrorSet *errset)
 
     if (vss_ctx.pVssbc) { /* already frozen */
         *num_vols = 0;
+        PRINT_DEBUG("finished, already frozen");
         return;
     }
 
@@ -449,6 +473,7 @@ void requester_freeze(int *num_vols, void *mountpoints, ErrorSet *errset)
         }
     }
 
+    PRINT_DEBUG("preparing for backup");
     hr = vss_ctx.pVssbc->PrepareForBackup(pAsync.replace());
     if (SUCCEEDED(hr)) {
         hr = WaitForAsync(pAsync);
@@ -472,6 +497,7 @@ void requester_freeze(int *num_vols, void *mountpoints, ErrorSet *errset)
      * CQGAVssProvider::CommitSnapshots will kick vss_ctx.hEventFrozen
      * after the applications and filesystems are frozen.
      */
+    PRINT_DEBUG("do snapshot set");
     hr = vss_ctx.pVssbc->DoSnapshotSet(&vss_ctx.pAsyncSnapshot);
     if (FAILED(hr)) {
         err_set(errset, hr, "failed to do snapshot set");
@@ -518,6 +544,7 @@ void requester_freeze(int *num_vols, void *mountpoints, ErrorSet *errset)
         *num_vols = vss_ctx.cFrozenVols = num_fixed_drives;
     }
 
+    PRINT_DEBUG("end successful");
     return;
 
 out:
@@ -528,11 +555,14 @@ out:
 out1:
     requester_cleanup();
     CoUninitialize();
+
+    PRINT_DEBUG_END;
 }
 
 
 void requester_thaw(int *num_vols, void *mountpints, ErrorSet *errset)
 {
+    PRINT_DEBUG_BEGIN;
     COMPointer<IVssAsync> pAsync;
 
     if (!vss_ctx.hEventThaw) {
@@ -541,6 +571,8 @@ void requester_thaw(int *num_vols, void *mountpints, ErrorSet *errset)
          * and no volumes must be frozen. We return without an error.
          */
         *num_vols = 0;
+        PRINT_DEBUG("finished, no volumes were frozen");
+
         return;
     }
 
@@ -597,4 +629,6 @@ void requester_thaw(int *num_vols, void *mountpints, ErrorSet *errset)
 
     CoUninitialize();
     StopService();
+
+    PRINT_DEBUG_END;
 }
-- 
2.34.1

Re: [PATCH 1/4] QGA VSS: Add wrapper to send log to debugger and stderr

[Philippe Mathieu-Daudé]

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

Re: [PATCH 2/4] QGA VSS: Replace 'fprintf(stderr' with PRINT_DEBUG

[Philippe Mathieu-Daudé]

On 5/7/23 16:12, Konstantin Kostiuk wrote:
> Signed-off-by: Konstantin Kostiuk <kkostiuk@redhat.com>
> ---
>   qga/vss-win32/install.cpp   | 13 +++++++------
>   qga/vss-win32/requester.cpp |  9 +++++----
>   2 files changed, 12 insertions(+), 10 deletions(-)
> 
> diff --git a/qga/vss-win32/install.cpp b/qga/vss-win32/install.cpp
> index ff93b08a9e..c10a397e51 100644
> --- a/qga/vss-win32/install.cpp
> +++ b/qga/vss-win32/install.cpp
> @@ -13,6 +13,7 @@
>   #include "qemu/osdep.h"
>   
>   #include "vss-common.h"
> +#include "vss-debug.h"
>   #ifdef HAVE_VSS_SDK
>   #include <vscoordint.h>
>   #else
> @@ -54,7 +55,7 @@ void errmsg(DWORD err, const char *text)
>                     FORMAT_MESSAGE_FROM_SYSTEM | FORMAT_MESSAGE_IGNORE_INSERTS,
>                     NULL, err, MAKELANGID(LANG_NEUTRAL, SUBLANG_DEFAULT),
>                     (char *)&msg, 0, NULL);
> -    fprintf(stderr, "%.*s. (Error: %lx) %s\n", len, text, err, msg);
> +    PRINT_DEBUG("%.*s. (Error: %lx) %s\n", len, text, err, msg);

PRINT_DEBUG() ends calling fprintf(stderr)...

Re: [PATCH 1/4] QGA VSS: Add wrapper to send log to debugger and stderr

[Konstantin Kostiuk]

[-- Attachment #1: Type: text/plain, Size: 1522 bytes --]

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
>
>

[-- Attachment #2: Type: text/html, Size: 2604 bytes --]

Re: [PATCH 2/4] QGA VSS: Replace 'fprintf(stderr' with PRINT_DEBUG

[Konstantin Kostiuk]

[-- Attachment #1: Type: text/plain, Size: 1384 bytes --]

On Wed, Jul 5, 2023 at 11:36 PM Philippe Mathieu-Daudé <philmd@linaro.org>
wrote:

> On 5/7/23 16:12, Konstantin Kostiuk wrote:
> > Signed-off-by: Konstantin Kostiuk <kkostiuk@redhat.com>
> > ---
> >   qga/vss-win32/install.cpp   | 13 +++++++------
> >   qga/vss-win32/requester.cpp |  9 +++++----
> >   2 files changed, 12 insertions(+), 10 deletions(-)
> >
> > diff --git a/qga/vss-win32/install.cpp b/qga/vss-win32/install.cpp
> > index ff93b08a9e..c10a397e51 100644
> > --- a/qga/vss-win32/install.cpp
> > +++ b/qga/vss-win32/install.cpp
> > @@ -13,6 +13,7 @@
> >   #include "qemu/osdep.h"
> >
> >   #include "vss-common.h"
> > +#include "vss-debug.h"
> >   #ifdef HAVE_VSS_SDK
> >   #include <vscoordint.h>
> >   #else
> > @@ -54,7 +55,7 @@ void errmsg(DWORD err, const char *text)
> >                     FORMAT_MESSAGE_FROM_SYSTEM |
> FORMAT_MESSAGE_IGNORE_INSERTS,
> >                     NULL, err, MAKELANGID(LANG_NEUTRAL, SUBLANG_DEFAULT),
> >                     (char *)&msg, 0, NULL);
> > -    fprintf(stderr, "%.*s. (Error: %lx) %s\n", len, text, err, msg);
> > +    PRINT_DEBUG("%.*s. (Error: %lx) %s\n", len, text, err, msg);
>
> PRINT_DEBUG() ends calling fprintf(stderr)...
>
>
Yes, PRINT_DEBUG calling fprintf(stderr).
This patch replaces fprintf(stderr call from errmsg with PRINT_DEBUG.

What do you mean by this comment?

[-- Attachment #2: Type: text/html, Size: 2065 bytes --]

Re: [PATCH 1/4] QGA VSS: Add wrapper to send log to debugger and stderr

[Philippe Mathieu-Daudé]

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()?

Re: [PATCH 1/4] QGA VSS: Add wrapper to send log to debugger and stderr

[Konstantin Kostiuk]

[-- Attachment #1: Type: text/plain, Size: 2491 bytes --]

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.

[-- Attachment #2: Type: text/html, Size: 4138 bytes --]

Re: [PATCH 1/4] QGA VSS: Add wrapper to send log to debugger and stderr

[Philippe Mathieu-Daudé]

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 :)