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.