September 2017 - Wine-devel

Re: [PATCH 2/3] msi: Store the current script in the package.
by Hans Leidekker 19 Sep '17

19 Sep '17

On Tue, 2017-09-19 at 00:23 -0500, Zebediah Figura wrote: > diff --git a/dlls/msi/custom.c b/dlls/msi/custom.c > index a3e32d9a24..b463ab659c 100644 > --- a/dlls/msi/custom.c > +++ b/dlls/msi/custom.c > @@ -1123,24 +1123,6 @@ static UINT HANDLE_CustomType53_54( MSIPACKAGE *package, const WCHAR *source, co > return wait_thread_handle( info ); > } > > -static BOOL action_type_matches_script( UINT type, UINT script ) > -{ > - switch (script) > - { > - case SCRIPT_NONE: > - return FALSE; > - case SCRIPT_INSTALL: > - return !(type & msidbCustomActionTypeCommit) && !(type & msidbCustomActionTypeRollback); > - case SCRIPT_COMMIT: > - return (type & msidbCustomActionTypeCommit); > - case SCRIPT_ROLLBACK: > - return (type & msidbCustomActionTypeRollback); > - default: > - ERR("unhandled script %u\n", script); > - } > - return FALSE; > -} > - > static UINT defer_custom_action( MSIPACKAGE *package, const WCHAR *action, UINT type ) > { > WCHAR *actiondata = msi_dup_property( package->db, action ); > @@ -1178,7 +1160,7 @@ static UINT defer_custom_action( MSIPACKAGE *package, const WCHAR *action, UINT > return ERROR_SUCCESS; > } > > -UINT ACTION_CustomAction( MSIPACKAGE *package, LPCWSTR action, UINT script ) > +UINT ACTION_CustomAction( MSIPACKAGE *package, LPCWSTR action ) > { > static const WCHAR query[] = { > 'S','E','L','E','C','T',' ','*',' ','F','R','O','M',' ', > @@ -1218,7 +1200,7 @@ UINT ACTION_CustomAction( MSIPACKAGE *package, LPCWSTR action, UINT script ) > if (type & msidbCustomActionTypeNoImpersonate) > WARN("msidbCustomActionTypeNoImpersonate not handled\n"); > > - if (!action_type_matches_script( type, script )) > + if (package->script == SCRIPT_NONE) > { This function was called with script other than SCRIPT_NONE before, so this changes the order of execution. It would need a test, and should be in a separate patch.

2 2

Re: [PATCH 1/3] msi: Remove a superfluous substructure and convert its arrays to lists.
by Hans Leidekker 19 Sep '17

19 Sep '17

On Tue, 2017-09-19 at 00:23 -0500, Zebediah Figura wrote: > +struct action > +{ > + struct list entry; > + WCHAR *action; > +}; > + > typedef struct tagMSIPACKAGE > { > MSIOBJECTHDR hdr; > @@ -403,7 +418,10 @@ typedef struct tagMSIPACKAGE > struct list mimes; > struct list appids; > > - struct tagMSISCRIPT *script; > + struct list script_actions[SCRIPT_MAX]; > + struct list unique_actions; > + BOOL ExecuteSequenceRun; > + UINT InWhatSequence; Why? The package structure is heavy on lists already, some of these should be moved to arrays instead...

2 1

Re: [PATCH] msi/tests: Factor out success checks into table building functions.
by Hans Leidekker 19 Sep '17

19 Sep '17

On Tue, 2017-09-19 at 00:23 -0500, Zebediah Figura wrote: > @@ -3457,62 +3474,28 @@ static void test_join(void) > hdb = create_db(); > ok( hdb, "failed to create db\n"); > > - r = create_component_table( hdb ); > - ok( r == ERROR_SUCCESS, "cannot create Component table: %d\n", r ); > - > - r = add_component_entry( hdb, "'zygomatic', 'malar', 'INSTALLDIR', 0, '', ''" ); > - ok( r == ERROR_SUCCESS, "cannot add component: %d\n", r ); Please add a line parameter to the add_* macros so we don't loose that information.

2 1

Re: [resend PATCH] d3d9/tests: Some tests for D3DPRESENTFLAG_LOCKABLE_BACKBUFFER mode
by Henri Verbeet 19 Sep '17

19 Sep '17

On 19 September 2017 at 08:00, Nikolay Sivov <nsivov(a)codeweavers.com> wrote: > Signed-off-by: Nikolay Sivov <nsivov(a)codeweavers.com> > --- > dlls/d3d9/tests/device.c | 91 ++++++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 91 insertions(+) > I think this would apply to d3d8 as well. > + hr = IDirect3DDevice9_GetBackBuffer(device, 0, 0, D3DBACKBUFFER_TYPE_MONO, &surface); > + ok(SUCCEEDED(hr), "Failed to get backbuffer, hr %#x.\n", hr); > + > + hr = IDirect3DSurface9_LockRect(surface, &lockrect, NULL, D3DLOCK_DISCARD); > +todo_wine > + ok(hr == D3DERR_INVALIDCALL, "Got unexpected hr %#x.\n", hr); ... > + hr = IDirect3DDevice9_Reset(device, &present_parameters); > + ok(SUCCEEDED(hr), "Failed to reset device, hr %#x.\n", hr); It's interesting that that works, actually. Since the LockRect() succeeds on Wine, the surface is mapped when the reset happen. That seems like the kind of thing that shouldn't be allowed.

2 1

Re: [PATCH] msvcrt: Add _vfprintf_l.
by Piotr Caban 18 Sep '17

18 Sep '17

Hi, On 09/18/17 03:10, Gijs Vermeulen wrote: > +/********************************************************************* > + * _vfprintf_l (MSVCRT.@) > + */ > +int CDECL MSVCRT__vfprintf_l(MSVCRT_FILE* file, const char *format, > + MSVCRT__locale_t locale, __ms_va_list valist) Please fix the indentation here. Except of that the patch looks good for me. Thanks, Piotr

2 1

Re: [PATCH] propsys: Add PropVariantToBoolean and tests
by Nikolay Sivov 18 Sep '17

18 Sep '17

On 18.09.2017 21:20, Fabian Maurer wrote: > Signed-off-by: Fabian Maurer <dark.shadow4(a)web.de> > --- > dlls/propsys/propsys.spec | 2 +- > dlls/propsys/propvar.c | 58 +++++++++++++++++ > dlls/propsys/tests/propsys.c | 147 +++++++++++++++++++++++++++++++++++++++++++ > include/propvarutil.h | 1 + > 4 files changed, 207 insertions(+), 1 deletion(-) > > diff --git a/dlls/propsys/propsys.spec b/dlls/propsys/propsys.spec > index 582308f5a0..86a5e696eb 100644 > --- a/dlls/propsys/propsys.spec > +++ b/dlls/propsys/propsys.spec > @@ -107,7 +107,7 @@ > @ stub PropVariantGetUInt32Elem > @ stub PropVariantGetUInt64Elem > @ stub PropVariantToBSTR > -@ stub PropVariantToBoolean > +@ stdcall PropVariantToBoolean(ptr ptr) > @ stub PropVariantToBooleanVector > @ stub PropVariantToBooleanVectorAlloc > @ stub PropVariantToBooleanWithDefault > diff --git a/dlls/propsys/propvar.c b/dlls/propsys/propvar.c > index ca3b421ddd..25cebfee02 100644 > --- a/dlls/propsys/propvar.c > +++ b/dlls/propsys/propvar.c > @@ -227,6 +227,64 @@ HRESULT WINAPI PropVariantToUInt64(REFPROPVARIANT propvarIn, ULONGLONG *ret) > return hr; > } > > +HRESULT WINAPI PropVariantToBoolean(REFPROPVARIANT propvarIn, BOOL *ret) > +{ > + LONGLONG res; > + HRESULT hr = S_OK; Initialization does not do anything. > + static const WCHAR wstr_true[] = {'t','r','u','e',0}; > + static const WCHAR wstr_false[] = {'f','a','l','s','e',0}; > + static const CHAR str_true[] = {'t','r','u','e',0}; > + static const CHAR str_false[] = {'f','a','l','s','e',0}; For single byte case you can use string literals right where you need them. > + > + TRACE("%p,%p\n", propvarIn, ret); > + > + *ret = 0; Is it correct to crash on NULL argument? > + > + switch(propvarIn->vt) > + { > + case VT_BOOL: > + *ret = propvarIn->u.boolVal == VARIANT_TRUE; > + return S_OK; > + > + case VT_LPWSTR: > + case VT_BSTR: > + if(!propvarIn->u.pwszVal) > + return DISP_E_TYPEMISMATCH; > + > + if(lstrcmpiW(propvarIn->u.pwszVal, wstr_true) == 0) > + { > + *ret = 1; > + return S_OK; > + } > + if(lstrcmpiW(propvarIn->u.pwszVal, wstr_false) == 0) > + { > + *ret = 0; > + return S_OK; > + } > + break; > + TRUE/FALSE are more common in this context than 0/1 I think. Same with tests. > + case VT_LPSTR: > + if(!propvarIn->u.pszVal) > + return DISP_E_TYPEMISMATCH; > + > + if(lstrcmpiA(propvarIn->u.pszVal, str_true) == 0) > + { > + *ret = 1; > + return S_OK; > + } > + if(lstrcmpiA(propvarIn->u.pszVal, str_false) == 0) > + { > + *ret = 0; > + return S_OK; > + } > + break; > + } > + > + hr = PROPVAR_ConvertNumber(propvarIn, 64, FALSE, &res); > + *ret = !!res; > + return hr; > +} > + > HRESULT WINAPI PropVariantToStringAlloc(REFPROPVARIANT propvarIn, WCHAR **ret) > { > WCHAR *res = NULL; > diff --git a/dlls/propsys/tests/propsys.c b/dlls/propsys/tests/propsys.c > index 776ccd0bfd..5f77f60e5f 100644 > --- a/dlls/propsys/tests/propsys.c > +++ b/dlls/propsys/tests/propsys.c > @@ -883,6 +883,152 @@ static void test_intconversions(void) > ok(llval == -7, "got wrong value %s\n", wine_dbgstr_longlong(llval)); > } > > +static void test_PropVariantToBoolean(void) > +{ > + PROPVARIANT propvar; > + BOOL val; > + HRESULT hr; > + static WCHAR str_0[] = {'0',0}; > + static WCHAR str_1[] = {'1',0}; > + static WCHAR str_7[] = {'7',0}; > + static WCHAR str_true[] = {'t','r','u','e',0}; > + static WCHAR str_true_case[] = {'t','R','U','e',0}; > + static WCHAR str_false[] = {'f','a','l','s','e',0}; > + static WCHAR str_true_space[] = {'t','r','u','e',' ',0}; > + static WCHAR str_yes[] = {'y','e','s',0}; > + static CHAR str_true_ansi[] = {'t','R','U','e',0}; > + static CHAR str_false_ansi[] = {'f','A','L','S','e',0}; > + static CHAR str_1_ansi[] = {'1',0}; > + > + /* test booleans */ > + > + propvar.vt = VT_BOOL; > + propvar.u.boolVal = 0; > + hr = PropVariantToBoolean(&propvar, &val); > + ok(hr == S_OK, "hr=%x\n", hr); > + ok(val == 0, "got wrong value %d\n", val); Better name for 0 in this context is VARIANT_FALSE. > + > + propvar.vt = VT_LPWSTR; > + propvar.u.pwszVal = 0; NULL.

2 2

Re: [PATCH] d3d9/tests: Some tests for D3DPRESENTFLAG_LOCKABLE_BACKBUFFER mode
by Marvin 18 Sep '17

18 Sep '17

Hi, While running your changed tests on Windows, I think I found new failures. Being a bot and all I'm not very good at pattern recognition, so I might be wrong, but could you please double-check? Full results can be found at https://testbot.winehq.org/JobDetails.pl?Key=33161 Your paranoid android. === build (build) === Make failed

2 1

Re: [v1 3/3] kernel32/tests: pipe: Added test for pipe security attributes
by Jacek Caban 18 Sep '17

18 Sep '17

Hi Jonathan, As I mentioned in the other mail, it would be interesting to add some more tests. As for those tests, I have a few comments bellow. On 12.09.2017 10:01, Jonathan Doron wrote: > Signed-off-by: Jonathan Doron <jond(a)wizery.com> > --- > dlls/kernel32/tests/pipe.c | 241 +++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 241 insertions(+) > > diff --git a/dlls/kernel32/tests/pipe.c b/dlls/kernel32/tests/pipe.c > index 0a6edd6..9b90ce9 100644 > --- a/dlls/kernel32/tests/pipe.c > +++ b/dlls/kernel32/tests/pipe.c > @@ -28,6 +28,7 @@ > #include "winternl.h" > #include "winioctl.h" > #include "wine/test.h" > +#include "sddl.h" > > #define PIPENAME "\\\\.\\PiPe\\tests_pipe.c" > > @@ -3018,6 +3019,245 @@ static void test_overlapped_transport(BOOL msg_mode, BOOL msg_read_mode) > CloseHandle(client); > } > > +static PSECURITY_DESCRIPTOR GetObjectSecDesc(HANDLE Object) > +{ > + ULONG RequiredLength; > + PSECURITY_DESCRIPTOR SecDesc = NULL; > + > + RequiredLength = 0; > + NtQuerySecurityObject(Object, > + GROUP_SECURITY_INFORMATION | OWNER_SECURITY_INFORMATION, > + NULL, 0, &RequiredLength); // Expected c0000023 > + if (!RequiredLength) > + goto cleanup; > + > + SecDesc = LocalAlloc(LMEM_FIXED, RequiredLength); Please use HeapAlloc(). > + if (!SecDesc) > + goto cleanup; > + memset(SecDesc, 0, RequiredLength); > + > + if (NtQuerySecurityObject(Object, > + GROUP_SECURITY_INFORMATION | OWNER_SECURITY_INFORMATION, > + SecDesc, RequiredLength, &RequiredLength)) { > + LocalFree(SecDesc); > + SecDesc = NULL; > + goto cleanup; > + } > + > +cleanup: > + return SecDesc; > +} Silent failures are not really good for tests. Please test NtQuerySecurityObject result with ok(). Same for other similar cases. > +static ULONG WINAPI ServerPipeThread(PVOID Param) > +{ > + ULONG RetVal; > + > + RetVal = ConnectNamedPipe((HANDLE)Param, NULL) ? ERROR_SUCCESS : GetLastError(); > + if ((RetVal == ERROR_PIPE_CONNECTED) || (RetVal == ERROR_SUCCESS)) > + RetVal = ERROR_SUCCESS; > + > + ExitThread(RetVal); > +} This is not really needed. You may just connect to file after it's created, skipping new thread and ConnectNamedPipe() call. Also there are styling issues. Please don't use C++ comments (//) in Wine and try to match style of existing code (eg. function names, variable names). Thanks, Jacek

1 0

Re: [v1 2/3] server: Named pipe client needs to use the server SD
by Jacek Caban 18 Sep '17

18 Sep '17

Hi Jonathan, On 12.09.2017 10:01, Jonathan Doron wrote: > +static struct security_descriptor *pipe_client_get_sd( struct object *obj ) > +{ > + struct pipe_client *client = (struct pipe_client *) obj; > + if (client->server) > + return default_get_sd((struct object *)client->server); > + return default_get_sd(obj); > +} > + > +static int pipe_client_set_sd( struct object *obj, const struct security_descriptor *sd, > + unsigned int set_info ) > +{ > + struct pipe_client *client = (struct pipe_client *) obj; > + if (client->server) > + return default_set_sd((struct object *)client->server, sd, set_info); > + return default_set_sd(obj, sd, set_info); > +} This looks suspicious to me, it will require more tests. Your tests from the other patch don't really show that we should use the same security descriptor. For example, we could just copy server security descriptor when creating pipe client (and have separated copies later) and it would pass tests as well. It would be interesting add a test that modifies server's security descriptor after client is created and see what happens to client. As for this approach, there are some problems with corner cases like what happens to client's security descriptor when server is destroyed. Thanks, Jacek

1 0

Re: [PATCH] strmiids: Add evr interface GUIDS
by Alexandre Julliard 18 Sep '17

18 Sep '17

Fabian Maurer <dark.shadow4(a)web.de> writes: > @@ -18,12 +18,7 @@ > * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301, USA > */ > > -import "unknwn.idl"; > -import "propidl.idl"; > import "mfidl.idl"; > -import "mftransform.idl"; > -import "mediaobj.idl"; > -import "strmif.idl"; These are included by the MS header, so we need to have them too. -- Alexandre Julliard julliard(a)winehq.org

2 3