Re: [PATCH 1/5] msi/tests: Add tests for external UI callback.

[Hans Leidekker]

On Sun, 2017-06-25 at 21:13 -0500, Zebediah Figura wrote:
> @@ -822,6 +823,8 @@ static UINT package_from_db(MSIHANDLE hdb, MSIHANDLE *handle)
>      CHAR szPackage[12];
>      MSIHANDLE hPackage;
> - CoInitialize(NULL);
> - sprintf(szPackage, "#%u", hdb);
>    res = MsiOpenPackageA(szPackage, &hPackage);
>    if (res != ERROR_SUCCESS)
>
> @@ -837,22 +840,39 @@ static UINT package_from_db(MSIHANDLE hdb, MSIHANDLE *handle)
>          return res;
>      }
> - CoUninitialize();
> -

Why do you need this?
> -static void create_test_file(const CHAR *name)
> +static void create_file_data(LPCSTR name, LPCSTR data, DWORD size)
>  {
>      HANDLE file;
>      DWORD written;
>  file = CreateFileA(name, GENERIC_WRITE, 0, NULL, CREATE_ALWAYS, 0, NULL);
>  ok(file != INVALID_HANDLE_VALUE, "Failure to open file %s\n", name);
> - WriteFile(file, name, strlen(name), &written, NULL);
> - if (file == INVALID_HANDLE_VALUE)
> -
>    return;
> -
> - WriteFile(file, data, strlen(data), &written, NULL);
> WriteFile(file, "\n", strlen("\n"), &written, NULL);
> -
> - if (size)
> - {
> -
>    SetFilePointer(file, size, NULL, FILE_BEGIN);
> -
>    SetEndOfFile(file);
> - }
> - CloseHandle(file);
>
> }

You're not using this in this patch set.

[PATCH 1/5] msi/tests: Add tests for external UI callback.

[Z Figura]

On 06/26/2017 04:07 AM, Hans Leidekker wrote:
> On Sun, 2017-06-25 at 21:13 -0500, Zebediah Figura wrote:
> > @@ -822,6 +823,8 @@ static UINT package_from_db(MSIHANDLE hdb, MSIHANDLE *handle)
> >       CHAR szPackage[12];
> >       MSIHANDLE hPackage;
> > - CoInitialize(NULL);
> > -
> > sprintf(szPackage, "#%u", hdb);
> > res = MsiOpenPackageA(szPackage, &hPackage);
> > if (res != ERROR_SUCCESS)
> >
> > @@ -837,22 +840,39 @@ static UINT package_from_db(MSIHANDLE hdb, MSIHANDLE *handle)
> >           return res;
> >       }
> > - CoUninitialize();
> > -
>
> Why do you need this?

The message tests fail on Windows > 8 if COM is not initialized. This is 
as per MSDN:

"Note  Initialize COM on the same thread before calling the 
MsiOpenPackage, MsiOpenPackageEx, or MsiOpenProduct function."
> > -static void create_test_file(const CHAR *name)
> > +static void create_file_data(LPCSTR name, LPCSTR data, DWORD size)
> >   {
> >       HANDLE file;
> >       DWORD written;
> >   file = CreateFileA(name, GENERIC_WRITE, 0, NULL, CREATE_ALWAYS, 0, NULL);
> >   ok(file != INVALID_HANDLE_VALUE, "Failure to open file %s\n", name);
> > - WriteFile(file, name, strlen(name), &written, NULL);
> > - if (file == INVALID_HANDLE_VALUE)
> > -
> >    return;
> > -
> > - WriteFile(file, data, strlen(data), &written, NULL);
> >  WriteFile(file, "\n", strlen("\n"), &written, NULL);
> > -
> > - if (size)
> > - {
> > -
> >    SetFilePointer(file, size, NULL, FILE_BEGIN);
> > -
> >    SetEndOfFile(file);
> > - }
> > -
> > CloseHandle(file);
> > }
>
> You're not using this in this patch set.

I use it below (line 261 of the patch) to set the codepage, which I do 
in order to test the correct setting of field 3 of the 
INSTALLMESSAGE_COMMONDATA messages.

[PATCH 1/5] msi/tests: Add tests for external UI callback.

[Hans Leidekker]

On Mon, 2017-06-26 at 09:17 -0500, Z Figura wrote:
> On 06/26/2017 04:07 AM, Hans Leidekker wrote:
> > On Sun, 2017-06-25 at 21:13 -0500, Zebediah Figura wrote:
> > > @@ -822,6 +823,8 @@ static UINT package_from_db(MSIHANDLE hdb, MSIHANDLE *handle)
> > >       CHAR szPackage[12];
> > >       MSIHANDLE hPackage;
> > > - CoInitialize(NULL);
> > > -
> > > sprintf(szPackage, "#%u", hdb);
> > > res = MsiOpenPackageA(szPackage, &hPackage);
> > > if (res != ERROR_SUCCESS)
> > >
> > > @@ -837,22 +840,39 @@ static UINT package_from_db(MSIHANDLE hdb, MSIHANDLE *handle)
> > >           return res;
> > >       }
> > > - CoUninitialize();
> > > -
> >
> > Why do you need this?
>
> The message tests fail on Windows > 8 if COM is not initialized. This is 
> as per MSDN:
>
> "Note  Initialize COM on the same thread before calling the 
> MsiOpenPackage, MsiOpenPackageEx, or MsiOpenProduct function."

Then it would be better to wrap them only for the message tests.
> > > -static void create_test_file(const CHAR *name)
> > > +static void create_file_data(LPCSTR name, LPCSTR data, DWORD size)
> > >   {
> > >       HANDLE file;
> > >       DWORD written;
> > >   file = CreateFileA(name, GENERIC_WRITE, 0, NULL, CREATE_ALWAYS, 0, NULL);
> > >   ok(file != INVALID_HANDLE_VALUE, "Failure to open file %s\n", name);
> > > - WriteFile(file, name, strlen(name), &written, NULL);
> > > - if (file == INVALID_HANDLE_VALUE)
> > > -
> > >    return;
> > > -
> > > - WriteFile(file, data, strlen(data), &written, NULL);
> > >  WriteFile(file, "\n", strlen("\n"), &written, NULL);
> > > -
> > > - if (size)
> > > - {
> > > -
> > >    SetFilePointer(file, size, NULL, FILE_BEGIN);
> > > -
> > >    SetEndOfFile(file);
> > > - }
> > > -
> > > CloseHandle(file);
> > > }
> >
> > You're not using this in this patch set.
>
> I use it below (line 261 of the patch) to set the codepage, which I do 
> in order to test the correct setting of field 3 of the 
> INSTALLMESSAGE_COMMONDATA messages.

You're calling it with size 0, so you're not using the code to explicitly
set the file size.

[PATCH 1/5] msi/tests: Add tests for external UI callback.

[Zebediah Figura]

Ah, I see. I'll fix these and the other patches.

On 06/26/2017 09:37 AM, Hans Leidekker wrote:
> On Mon, 2017-06-26 at 09:17 -0500, Z Figura wrote:
> > On 06/26/2017 04:07 AM, Hans Leidekker wrote:
> > > On Sun, 2017-06-25 at 21:13 -0500, Zebediah Figura wrote:
> > > > @@ -822,6 +823,8 @@ static UINT package_from_db(MSIHANDLE hdb, MSIHANDLE *handle)
> > > >        CHAR szPackage[12];
> > > >        MSIHANDLE hPackage;
> > > > - CoInitialize(NULL);
> > > > -
> > > >  sprintf(szPackage, "#%u", hdb);
> > > >  res = MsiOpenPackageA(szPackage, &hPackage);
> > > >  if (res != ERROR_SUCCESS)
> > > >
> > > > @@ -837,22 +840,39 @@ static UINT package_from_db(MSIHANDLE hdb, MSIHANDLE *handle)
> > > >            return res;
> > > >        }
> > > > - CoUninitialize();
> > > > -
> > >
> > > Why do you need this?
> >
> > The message tests fail on Windows > 8 if COM is not initialized. This is
> > as per MSDN:
> >
> > "Note  Initialize COM on the same thread before calling the
> > MsiOpenPackage, MsiOpenPackageEx, or MsiOpenProduct function."
>
> Then it would be better to wrap them only for the message tests.
> > > > -static void create_test_file(const CHAR *name)
> > > > +static void create_file_data(LPCSTR name, LPCSTR data, DWORD size)
> > > >    {
> > > >        HANDLE file;
> > > >        DWORD written;
> > > >    file = CreateFileA(name, GENERIC_WRITE, 0, NULL, CREATE_ALWAYS, 0, NULL);
> > > >    ok(file != INVALID_HANDLE_VALUE, "Failure to open file %s\n", name);
> > > > - WriteFile(file, name, strlen(name), &written, NULL);
> > > > - if (file == INVALID_HANDLE_VALUE)
> > > > -
> > > >    return;
> > > > -
> > > > - WriteFile(file, data, strlen(data), &written, NULL);
> > > >   WriteFile(file, "\n", strlen("\n"), &written, NULL);
> > > > -
> > > > - if (size)
> > > > - {
> > > > -
> > > >    SetFilePointer(file, size, NULL, FILE_BEGIN);
> > > > -
> > > >    SetEndOfFile(file);
> > > > - }
> > > > -
> > > >  CloseHandle(file);
> > > > }
> > >
> > > You're not using this in this patch set.
> >
> > I use it below (line 261 of the patch) to set the codepage, which I do
> > in order to test the correct setting of field 3 of the
> > INSTALLMESSAGE_COMMONDATA messages.
>
> You're calling it with size 0, so you're not using the code to explicitly
> set the file size.