[1/3] qmgr: Only have one BackgroundCopyManager per system.
Dan Hipschman
dsh at linux.ucla.edu
Wed Mar 5 20:00:52 CST 2008
The current implementation isn't right. There is only one manager per
system. This also adds a test for that which passes on Windows. We
need a bunch of critical sections now (just around access to the job
list at the moment).
The test spawns a separate process that adds a job, then the parent
process makes sure it can see it.
---
dlls/qmgr/enum_jobs.c | 4 ++
dlls/qmgr/qmgr.c | 54 ++++++++++++++----------
dlls/qmgr/qmgr.h | 1 +
dlls/qmgr/tests/qmgr.c | 106 ++++++++++++++++++++++++++++++++++++++++++++++-
4 files changed, 139 insertions(+), 26 deletions(-)
diff --git a/dlls/qmgr/enum_jobs.c b/dlls/qmgr/enum_jobs.c
index e35730a..4a1e5c8 100644
--- a/dlls/qmgr/enum_jobs.c
+++ b/dlls/qmgr/enum_jobs.c
@@ -181,6 +181,8 @@ HRESULT EnumBackgroundCopyJobsConstructor(LPVOID *ppObj,
/* Create array of jobs */
This->indexJobs = 0;
+
+ EnterCriticalSection(qmgr->jobList_cs);
This->numJobs = list_count(&qmgr->jobs);
if (0 < This->numJobs)
@@ -189,6 +191,7 @@ HRESULT EnumBackgroundCopyJobsConstructor(LPVOID *ppObj,
This->numJobs * sizeof *This->jobs);
if (!This->jobs)
{
+ LeaveCriticalSection(qmgr->jobList_cs);
HeapFree(GetProcessHeap(), 0, This);
return E_OUTOFMEMORY;
}
@@ -203,6 +206,7 @@ HRESULT EnumBackgroundCopyJobsConstructor(LPVOID *ppObj,
IBackgroundCopyJob_AddRef(iJob);
This->jobs[i++] = iJob;
}
+ LeaveCriticalSection(qmgr->jobList_cs);
*ppObj = &This->lpVtbl;
return S_OK;
diff --git a/dlls/qmgr/qmgr.c b/dlls/qmgr/qmgr.c
index fdfd30c..dbcbab4 100644
--- a/dlls/qmgr/qmgr.c
+++ b/dlls/qmgr/qmgr.c
@@ -26,13 +26,16 @@ WINE_DEFAULT_DEBUG_CHANNEL(qmgr);
/* Destructor for instances of background copy manager */
static void BackgroundCopyManagerDestructor(BackgroundCopyManagerImpl *This)
{
- BackgroundCopyJobImpl *job;
+ BackgroundCopyJobImpl *job, *job2;
TRACE("%p\n", This);
- LIST_FOR_EACH_ENTRY(job, &This->jobs, BackgroundCopyJobImpl, entryFromQmgr)
+ EnterCriticalSection(This->jobList_cs);
+ LIST_FOR_EACH_ENTRY_SAFE(job, job2, &This->jobs, BackgroundCopyJobImpl, entryFromQmgr)
+ {
+ list_remove(&job->entryFromQmgr);
job->lpVtbl->Release((IBackgroundCopyJob *) job);
-
- HeapFree(GetProcessHeap(), 0, This);
+ }
+ LeaveCriticalSection(This->jobList_cs);
}
/* Add a reference to the iface pointer */
@@ -40,11 +43,8 @@ static ULONG WINAPI BITS_IBackgroundCopyManager_AddRef(
IBackgroundCopyManager* iface)
{
BackgroundCopyManagerImpl * This = (BackgroundCopyManagerImpl *)iface;
- ULONG ref;
-
- TRACE("\n");
-
- ref = InterlockedIncrement(&This->ref);
+ ULONG ref = InterlockedIncrement(&This->ref);
+ TRACE("-> %u\n", ref);
return ref;
}
@@ -75,11 +75,8 @@ static ULONG WINAPI BITS_IBackgroundCopyManager_Release(
IBackgroundCopyManager* iface)
{
BackgroundCopyManagerImpl * This = (BackgroundCopyManagerImpl *)iface;
- ULONG ref;
-
- TRACE("\n");
-
- ref = InterlockedDecrement(&This->ref);
+ ULONG ref = InterlockedDecrement(&This->ref);
+ TRACE("-> %u\n", ref);
if (ref == 0)
{
BackgroundCopyManagerDestructor(This);
@@ -109,7 +106,9 @@ static HRESULT WINAPI BITS_IBackgroundCopyManager_CreateJob(
/* Add a reference to the job to job list */
IBackgroundCopyJob_AddRef(*ppJob);
job = (BackgroundCopyJobImpl *) *ppJob;
+ EnterCriticalSection(This->jobList_cs);
list_add_head(&This->jobs, &job->entryFromQmgr);
+ LeaveCriticalSection(This->jobList_cs);
return S_OK;
}
@@ -153,6 +152,22 @@ static const IBackgroundCopyManagerVtbl BITS_IBackgroundCopyManager_Vtbl =
BITS_IBackgroundCopyManager_GetErrorDescription
};
+static CRITICAL_SECTION globalJobList_cs;
+static CRITICAL_SECTION_DEBUG globalJobList_cs_debug =
+{
+ 0, 0, &globalJobList_cs,
+ { &globalJobList_cs_debug.ProcessLocksList,
+ &globalJobList_cs_debug.ProcessLocksList },
+ 0, 0, { (DWORD_PTR)(__FILE__ ": globalJobList_cs") }
+};
+static CRITICAL_SECTION globalJobList_cs = { &globalJobList_cs_debug, -1, 0, 0, 0, 0 };
+static BackgroundCopyManagerImpl globalMgr = {
+ &BITS_IBackgroundCopyManager_Vtbl,
+ &globalJobList_cs,
+ 0,
+ LIST_INIT(globalMgr.jobs)
+};
+
/* Constructor for instances of background copy manager */
HRESULT BackgroundCopyManagerConstructor(IUnknown *pUnkOuter, LPVOID *ppObj)
{
@@ -160,15 +175,8 @@ HRESULT BackgroundCopyManagerConstructor(IUnknown *pUnkOuter, LPVOID *ppObj)
TRACE("(%p,%p)\n", pUnkOuter, ppObj);
- This = HeapAlloc(GetProcessHeap(), 0, sizeof(*This));
- if (!This)
- {
- return E_OUTOFMEMORY;
- }
-
- This->lpVtbl = &BITS_IBackgroundCopyManager_Vtbl;
- This->ref = 1;
- list_init(&This->jobs);
+ This = &globalMgr;
+ IBackgroundCopyManager_AddRef((IBackgroundCopyManager *) This);
*ppObj = &This->lpVtbl;
return S_OK;
diff --git a/dlls/qmgr/qmgr.h b/dlls/qmgr/qmgr.h
index f40b1a5..5227120 100644
--- a/dlls/qmgr/qmgr.h
+++ b/dlls/qmgr/qmgr.h
@@ -78,6 +78,7 @@ typedef struct
typedef struct
{
const IBackgroundCopyManagerVtbl *lpVtbl;
+ CRITICAL_SECTION *jobList_cs;
LONG ref;
struct list jobs;
} BackgroundCopyManagerImpl;
diff --git a/dlls/qmgr/tests/qmgr.c b/dlls/qmgr/tests/qmgr.c
index 07af527..a641951 100644
--- a/dlls/qmgr/tests/qmgr.c
+++ b/dlls/qmgr/tests/qmgr.c
@@ -26,6 +26,8 @@
#include "initguid.h"
#include "bits.h"
+WCHAR progname[MAX_PATH];
+
static void
test_CreateInstance(void)
{
@@ -130,11 +132,109 @@ static void test_EnumJobs(void)
IBackgroundCopyManager_Release(manager);
}
+static void run_child(WCHAR *secret)
+{
+ static const WCHAR format[] = {'%','s',' ','q','m','g','r',' ','%','s', 0};
+ WCHAR cmdline[MAX_PATH];
+ PROCESS_INFORMATION info;
+ STARTUPINFOW startup;
+
+ memset(&startup, 0, sizeof startup);
+ startup.cb = sizeof startup;
+
+ wsprintfW(cmdline, format, progname, secret);
+ ok(CreateProcessW(NULL, cmdline, NULL, NULL, FALSE, 0L, NULL, NULL, &startup, &info), "CreateProcess\n");
+ winetest_wait_child_process(info.hProcess);
+ ok(CloseHandle(info.hProcess), "CloseHandle\n");
+ ok(CloseHandle(info.hThread), "CloseHandle\n");
+}
+
+static void do_child(const char *secretA)
+{
+ WCHAR secretW[MAX_PATH];
+ IBackgroundCopyManager *manager = NULL;
+ GUID id;
+ IBackgroundCopyJob *job;
+ HRESULT hres;
+ hres = CoCreateInstance(&CLSID_BackgroundCopyManager, NULL,
+ CLSCTX_LOCAL_SERVER, &IID_IBackgroundCopyManager,
+ (void **) &manager);
+ if(hres != S_OK)
+ {
+ skip("Unable to create bits instance required for test.\n");
+ return;
+ }
+
+ MultiByteToWideChar(CP_ACP, 0, secretA, -1, secretW, MAX_PATH);
+ hres = IBackgroundCopyManager_CreateJob(manager, secretW,
+ BG_JOB_TYPE_DOWNLOAD, &id, &job);
+ ok(hres == S_OK, "CreateJob in child process\n");
+ IBackgroundCopyJob_Release(job);
+ IBackgroundCopyManager_Release(manager);
+}
+
+static void test_globalness(void)
+{
+ static const WCHAR format[] = {'t','e','s','t','_','%','u', 0};
+ WCHAR secretName[MAX_PATH];
+ IEnumBackgroundCopyJobs* enumJobs;
+ IBackgroundCopyManager *manager = NULL;
+ HRESULT hres;
+ hres = CoCreateInstance(&CLSID_BackgroundCopyManager, NULL,
+ CLSCTX_LOCAL_SERVER, &IID_IBackgroundCopyManager,
+ (void **) &manager);
+ if(hres != S_OK)
+ {
+ skip("Unable to create bits instance required for test.\n");
+ return;
+ }
+
+ wsprintfW(secretName, format, GetTickCount());
+ run_child(secretName);
+
+ hres = IBackgroundCopyManager_EnumJobs(manager, 0, &enumJobs);
+ ok(hres == S_OK, "EnumJobs failed: %08x\n", hres);
+ if(hres != S_OK)
+ skip("Unable to create job enumerator.\n");
+ else
+ {
+ ULONG i, n;
+ IBackgroundCopyJob *job;
+ BOOL found = FALSE;
+
+ hres = IEnumBackgroundCopyJobs_GetCount(enumJobs, &n);
+ for (i = 0; i < n && !found; ++i)
+ {
+ LPWSTR name;
+ IEnumBackgroundCopyJobs_Next(enumJobs, 1, &job, NULL);
+ IBackgroundCopyJob_GetDisplayName(job, &name);
+ if (lstrcmpW(name, secretName) == 0)
+ found = TRUE;
+ CoTaskMemFree(name);
+ IBackgroundCopyJob_Release(job);
+ }
+ hres = IEnumBackgroundCopyJobs_Release(enumJobs);
+ ok(found, "Adding a job in another process failed\n");
+ }
+
+ IBackgroundCopyManager_Release(manager);
+}
+
START_TEST(qmgr)
{
+ char **argv;
+ int argc = winetest_get_mainargs(&argv);
+ MultiByteToWideChar(CP_ACP, 0, argv[0], -1, progname, MAX_PATH);
+
CoInitialize(NULL);
- test_CreateInstance();
- test_CreateJob();
- test_EnumJobs();
+ if (argc == 3)
+ do_child(argv[2]);
+ else
+ {
+ test_CreateInstance();
+ test_CreateJob();
+ test_EnumJobs();
+ test_globalness();
+ }
CoUninitialize();
}
More information about the wine-patches
mailing list