[PATCH v2 0/2] MR6827: msi: Fix MsiSummaryInfoPersist not saving data to disk
Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=55859 When a db is transacted, we need to call `Commit` to actually save the changes, but tests show this is not how msi works, so I changed `MsiGetSummaryInformationW` to open the db directly instead. -- v2: msi: Fix MsiSummaryInfoPersist not saving data to disk msi: Add more tests for MsiSummaryInfoPersist https://gitlab.winehq.org/wine/wine/-/merge_requests/6827
From: Fabian Maurer <dark.shadow4(a)web.de> --- dlls/msi/tests/suminfo.c | 107 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 107 insertions(+) diff --git a/dlls/msi/tests/suminfo.c b/dlls/msi/tests/suminfo.c index 59b56a2cbba..6a8dba7ddd7 100644 --- a/dlls/msi/tests/suminfo.c +++ b/dlls/msi/tests/suminfo.c @@ -278,6 +278,113 @@ static void test_suminfo(void) r = MsiCloseHandle(hsuminfo); ok(r == ERROR_SUCCESS, "MsiCloseHandle failed\n"); + /* try persisting on transacted msi */ + r = MsiOpenDatabaseW(msifileW, MSIDBOPEN_TRANSACT, &hdb); + ok(r == ERROR_SUCCESS, "MsiOpenDatabase failed\n"); + + r = MsiGetSummaryInformationA(hdb, NULL, 1, &hsuminfo); + ok(r == ERROR_SUCCESS, "MsiGetSummaryInformation wrong error\n"); + + r = MsiSummaryInfoSetPropertyA(hsuminfo, PID_AUTHOR, VT_LPSTR, 1, &ft, "Fabian"); + ok(r == ERROR_SUCCESS, "MsiSummaryInfoSetProperty wrong error\n"); + + r = MsiSummaryInfoPersist(hsuminfo); + ok(r == ERROR_SUCCESS, "MsiSummaryInfoPersist wrong error %u\n", r); + + r = MsiCloseHandle(hsuminfo); + ok(r == ERROR_SUCCESS, "MsiCloseHandle failed\n"); + + r = MsiCloseHandle(hdb); + ok(r == ERROR_SUCCESS, "MsiCloseHandle failed\n"); + + r = MsiOpenDatabaseW(msifileW, MSIDBOPEN_READONLY, &hdb); + ok(r == ERROR_SUCCESS, "MsiOpenDatabase failed\n"); + + r = MsiGetSummaryInformationA(hdb, NULL, 1, &hsuminfo); + ok(r == ERROR_SUCCESS, "MsiGetSummaryInformation wrong error\n"); + + sz = 0x10; + strcpy(buf,"x"); + r = MsiSummaryInfoGetPropertyA(hsuminfo, PID_AUTHOR, &type, NULL, NULL, buf, &sz); + ok(r == ERROR_SUCCESS, "MsiSummaryInfoGetPropertyA wrong error\n"); + ok(!strcmp(buf,"Mike"), "buffer was wrong: %s\n", buf); + + r = MsiCloseHandle(hsuminfo); + ok(r == ERROR_SUCCESS, "MsiCloseHandle failed\n"); + + r = MsiCloseHandle(hdb); + ok(r == ERROR_SUCCESS, "MsiCloseHandle failed\n"); + + /* try persisting on direct msi */ + r = MsiOpenDatabaseW(msifileW, MSIDBOPEN_DIRECT, &hdb); + ok(r == ERROR_SUCCESS, "MsiOpenDatabase failed\n"); + + r = MsiGetSummaryInformationA(hdb, NULL, 1, &hsuminfo); + ok(r == ERROR_SUCCESS, "MsiGetSummaryInformation wrong error\n"); + + r = MsiSummaryInfoSetPropertyA(hsuminfo, PID_AUTHOR, VT_LPSTR, 1, &ft, "Fabian"); + ok(r == ERROR_SUCCESS, "MsiSummaryInfoSetProperty wrong error\n"); + + r = MsiSummaryInfoPersist(hsuminfo); + ok(r == ERROR_SUCCESS, "MsiSummaryInfoPersist wrong error %u\n", r); + + r = MsiCloseHandle(hsuminfo); + ok(r == ERROR_SUCCESS, "MsiCloseHandle failed\n"); + + r = MsiCloseHandle(hdb); + ok(r == ERROR_SUCCESS, "MsiCloseHandle failed\n"); + + r = MsiOpenDatabaseW(msifileW, MSIDBOPEN_READONLY, &hdb); + ok(r == ERROR_SUCCESS, "MsiOpenDatabase failed\n"); + + r = MsiGetSummaryInformationA(hdb, NULL, 1, &hsuminfo); + ok(r == ERROR_SUCCESS, "MsiGetSummaryInformation wrong error\n"); + + sz = 0x10; + strcpy(buf,"x"); + r = MsiSummaryInfoGetPropertyA(hsuminfo, PID_AUTHOR, &type, NULL, NULL, buf, &sz); + ok(r == ERROR_SUCCESS, "MsiSummaryInfoGetPropertyA wrong error\n"); + ok(!strcmp(buf,"Fabian"), "buffer was wrong: %s\n", buf); + + r = MsiCloseHandle(hsuminfo); + ok(r == ERROR_SUCCESS, "MsiCloseHandle failed\n"); + + r = MsiCloseHandle(hdb); + ok(r == ERROR_SUCCESS, "MsiCloseHandle failed\n"); + + /* try persisting on indirectly opened msi */ + r = MsiGetSummaryInformationA(0, msifile, 2, &hsuminfo); + ok(r == ERROR_SUCCESS, "MsiGetSummaryInformation wrong error %u\n", r); + + r = MsiSummaryInfoSetPropertyA(hsuminfo, PID_AUTHOR, VT_LPSTR, 1, &ft, "Fabian2"); + ok(r == ERROR_SUCCESS, "MsiSummaryInfoSetProperty wrong error\n"); + + r = MsiSummaryInfoPersist(hsuminfo); + ok(r == ERROR_SUCCESS, "MsiSummaryInfoPersist wrong error %u\n", r); + + r = MsiCloseHandle(hsuminfo); + ok(r == ERROR_SUCCESS, "MsiCloseHandle failed\n"); + + r = MsiOpenDatabaseW(msifileW, MSIDBOPEN_READONLY, &hdb); + ok(r == ERROR_SUCCESS, "MsiOpenDatabase failed\n"); + + r = MsiGetSummaryInformationA(hdb, NULL, 1, &hsuminfo); + ok(r == ERROR_SUCCESS, "MsiGetSummaryInformation wrong error\n"); + + sz = 0x10; + strcpy(buf,"x"); + r = MsiSummaryInfoGetPropertyA(hsuminfo, PID_AUTHOR, &type, NULL, NULL, buf, &sz); + ok(r == ERROR_SUCCESS, "MsiSummaryInfoGetPropertyA wrong error\n"); + todo_wine + ok(!strcmp(buf,"Fabian2"), "buffer was wrong: %s\n", buf); + + r = MsiCloseHandle(hsuminfo); + ok(r == ERROR_SUCCESS, "MsiCloseHandle failed\n"); + + r = MsiCloseHandle(hdb); + ok(r == ERROR_SUCCESS, "MsiCloseHandle failed\n"); + + /* Cleanup */ r = DeleteFileA(msifile); ok(r, "DeleteFile failed\n"); } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/6827
From: Fabian Maurer <dark.shadow4(a)web.de> Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=55859 --- dlls/msi/suminfo.c | 2 +- dlls/msi/tests/suminfo.c | 1 - 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/dlls/msi/suminfo.c b/dlls/msi/suminfo.c index b1934cf41c7..8670904c59b 100644 --- a/dlls/msi/suminfo.c +++ b/dlls/msi/suminfo.c @@ -515,7 +515,7 @@ UINT WINAPI MsiGetSummaryInformationW( MSIHANDLE hDatabase, const WCHAR *szDatab if( szDatabase && szDatabase[0] ) { - LPCWSTR persist = uiUpdateCount ? MSIDBOPEN_TRANSACT : MSIDBOPEN_READONLY; + LPCWSTR persist = uiUpdateCount ? MSIDBOPEN_DIRECT : MSIDBOPEN_READONLY; ret = MSI_OpenDatabaseW( szDatabase, persist, &db ); if( ret != ERROR_SUCCESS ) diff --git a/dlls/msi/tests/suminfo.c b/dlls/msi/tests/suminfo.c index 6a8dba7ddd7..269d3529f5a 100644 --- a/dlls/msi/tests/suminfo.c +++ b/dlls/msi/tests/suminfo.c @@ -375,7 +375,6 @@ static void test_suminfo(void) strcpy(buf,"x"); r = MsiSummaryInfoGetPropertyA(hsuminfo, PID_AUTHOR, &type, NULL, NULL, buf, &sz); ok(r == ERROR_SUCCESS, "MsiSummaryInfoGetPropertyA wrong error\n"); - todo_wine ok(!strcmp(buf,"Fabian2"), "buffer was wrong: %s\n", buf); r = MsiCloseHandle(hsuminfo); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/6827
Hi, It looks like your patch introduced the new failures shown below. Please investigate and fix them before resubmitting your patch. If they are not new, fixing them anyway would help a lot. Otherwise please ask for the known failures list to be updated. The tests also ran into some preexisting test failures. If you know how to fix them that would be helpful. See the TestBot job for the details: The full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=149751 Your paranoid android. === debian11b (64 bit WoW report) === user32: win.c:4070: Test failed: Expected active window 00000000022B0146, got 0000000000000000. win.c:4071: Test failed: Expected focus window 00000000022B0146, got 0000000000000000.
On Tue Nov 19 01:42:52 2024 +0000, Hans Leidekker wrote:
When a db is transacted, we need to call `Commit` to actually save the changes, but tests show this is not how msi works, so I changed `MsiGetSummaryInformationW` to open the db directly instead. Your test shows that this is not how it works for a database that is opened by MsiGetSummaryInformationW(). The case where a transacted database handle is passed may work correctly but your tests don't cover that. Maybe you could add a test for that and update the commit message to reflect this nuance? Not sure what you mean, my first test checked what happens if you try to persist on a transacted handle. I added another test for persisting on a directly opened database for completeness.
What exactly should I change here? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6827#note_87951
On Tue Nov 19 01:42:52 2024 +0000, Fabian Maurer wrote:
Not sure what you mean, my first test checked what happens if you try to persist on a transacted handle. I added another test for persisting on a directly opened database for completeness. What exactly should I change here? Your test shows that calling MsiSummaryInfoPersist() has no effect if the database is transacted. That makes sense because changes should only be stored when MsiDatabaseCommit() is called. It would be nice if you could add a test that shows this. Then the commit message 'Fix MsiSummaryInfoPersist not saving data to disk' should be more specific. Your patch only handles the case where MsiGetSummaryInformationW() opens the database.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/6827#note_87972
participants (4)
-
Fabian Maurer -
Fabian Maurer (＠DarkShadow44)
-
Hans Leidekker (＠hans)
-
Marvin