April 2017 - Wine-Devel - list.winehq.org

Re: [2/3] wsdapi/tests: Add tests for Register/UnRegisterNotificationSink
by Nikolay Sivov April 19, 2017

April 19, 2017

Don't know if that's important, but this does not test a case when RegisterNotificationSink is called on already registered sink.

3 3

Re: [PATCH] mfplat: Add MFCreateAttributes stub
by Nikolay Sivov April 19, 2017

April 19, 2017

Does this make some application work, without providing actual interface implementation?

3 2

Re: [v2 PATCH 2/2] wined3d: Update only dirty regions in wined3d_device_update_texture().
by Marvin April 19, 2017

April 19, 2017

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=30251 Your paranoid android. === w1064 (32 bit visual) === visual.c:7992: Test failed: Got unexpected color 0x00007580 for quad 2 (different colors).

1 0

Re: [PATCH 2/3] user32: Switch to normal window drawing on SetLayeredWindowAttributes call
by Ken Thomases April 19, 2017

April 19, 2017

On Apr 18, 2017, at 11:43 AM, Piotr Caban <piotr(a)codeweavers.com> wrote: > > Signed-off-by: Piotr Caban <piotr(a)codeweavers.com> > --- > dlls/user32/win.c | 6 +++++- > dlls/winemac.drv/window.c | 2 ++ > dlls/winex11.drv/window.c | 2 ++ > 3 files changed, 9 insertions(+), 1 deletion(-) The Mac driver part of this looks OK to me. -Ken

1 0

Re: [PATCH 1/3] shell32: Avoid leaking icon handles.
by Andrew Eikum April 19, 2017

April 19, 2017

FWIW you're mixing up indent style in a function that is mostly consistently (if bizarrely) indented. Andrew On Wed, Apr 19, 2017 at 01:13:39PM +0100, Huw Davies wrote: > Signed-off-by: Huw Davies <huw(a)codeweavers.com> > --- > dlls/shell32/iconcache.c | 12 ++++++++++-- > 1 file changed, 10 insertions(+), 2 deletions(-) > > diff --git a/dlls/shell32/iconcache.c b/dlls/shell32/iconcache.c > index c3284a27d6..7beb47967d 100644 > --- a/dlls/shell32/iconcache.c > +++ b/dlls/shell32/iconcache.c > @@ -305,10 +305,12 @@ static INT SIC_IconAppend (LPCWSTR sSourceFile, INT dwSourceIndex, HICON hSmallI > * gets small/big icon by number from a file > */ > static INT SIC_LoadIcon (LPCWSTR sSourceFile, INT dwSourceIndex, DWORD dwFlags) > -{ HICON hiconLarge=0; > +{ > + HICON hiconLarge=0; > HICON hiconSmall=0; > HICON hiconLargeShortcut; > HICON hiconSmallShortcut; > + int ret; > > PrivateExtractIconsW( sSourceFile, dwSourceIndex, GetSystemMetrics(SM_CXICON), > GetSystemMetrics(SM_CYICON), &hiconLarge, 0, 1, 0 ); > @@ -327,6 +329,8 @@ static INT SIC_LoadIcon (LPCWSTR sSourceFile, INT dwSourceIndex, DWORD dwFlags) > hiconSmallShortcut = SIC_OverlayShortcutImage(hiconSmall, FALSE); > if (NULL != hiconLargeShortcut && NULL != hiconSmallShortcut) > { > + DestroyIcon( hiconLarge ); > + DestroyIcon( hiconSmall ); > hiconLarge = hiconLargeShortcut; > hiconSmall = hiconSmallShortcut; > } > @@ -339,8 +343,12 @@ static INT SIC_LoadIcon (LPCWSTR sSourceFile, INT dwSourceIndex, DWORD dwFlags) > } > } > > - return SIC_IconAppend (sSourceFile, dwSourceIndex, hiconSmall, hiconLarge, dwFlags); > + ret = SIC_IconAppend( sSourceFile, dwSourceIndex, hiconSmall, hiconLarge, dwFlags ); > + DestroyIcon( hiconLarge ); > + DestroyIcon( hiconSmall ); > + return ret; > } > + > /***************************************************************************** > * SIC_Initialize [internal] > */ > -- > 2.12.0 > > >

2 1

Re: [6/6] secur32: Add support for SECPKG_ATTR_KEY_INFO (GnuTLS only).
by Jacek Caban April 19, 2017

April 19, 2017

Hi Akihiro, Thanks for working on that. I have a few comments to the patch. On 17.04.2017 16:26, Akihiro Sagawa wrote: > diff --git a/dlls/secur32/schannel.c b/dlls/secur32/schannel.c > index 6914837..317686f 100644 > --- a/dlls/secur32/schannel.c > +++ b/dlls/secur32/schannel.c > @@ -54,11 +54,14 @@ struct schan_handle > enum schan_handle_type type; > }; > > +#define SCHAN_KEY_NAME_MAX 8 > struct schan_context > { > schan_imp_session session; > ULONG req_ctx_attr; > const CERT_CONTEXT *cert; > + SEC_WCHAR key_signature_name[SCHAN_KEY_NAME_MAX]; > + SEC_WCHAR key_encrypt_name[SCHAN_KEY_NAME_MAX]; Those should not be needed. You could for example store both WCHAR and char strings inside alg_name_map. > }; > > static struct schan_handle *schan_handle_table; > @@ -962,6 +965,53 @@ static SECURITY_STATUS SEC_ENTRY schan_InitializeSecurityContextA( > return ret; > } > > +struct schan_alg_id_name { > + ALG_ID alg_id; > + const char* name; > +}; > + > +static int comp_alg_id(const void *a, const void *b) > +{ > + const struct schan_alg_id_name *lhs = a; > + const struct schan_alg_id_name *rhs = b; > + return (int)lhs->alg_id - (int)rhs->alg_id; > +} > + > +static void fill_alg_name(ALG_ID id, void *buffer, BOOL wide) If you change the patch as I suggested above, this could be something like: static void *get_alg_name(ALF_ID id, BOOL wide) > +{ > + static const struct schan_alg_id_name alg_name_map[] = { > + { CALG_ECDSA, "ECDSA" }, > + { CALG_RSA_SIGN, "RSA" }, > + { CALG_DES, "DES" }, > + { CALG_RC2, "RC2" }, > + { CALG_3DES, "3DES" }, > + { CALG_AES_128, "AES" }, > + { CALG_AES_192, "AES" }, > + { CALG_AES_256, "AES" }, > + { CALG_RC4, "RC4" }, > + }; > + const struct schan_alg_id_name *res; > + struct schan_alg_id_name key; > + const char *name; > + > + key.alg_id = id; > + res = bsearch(&key, alg_name_map, > + sizeof(alg_name_map)/sizeof(alg_name_map[0]), sizeof(alg_name_map[0]), > + comp_alg_id); > + if (res) > + name = res->name; > + else > + { > + TRACE("Unknown ALG_ID %04x\n", id); > + name = "???"; > + } A FIXME would would be nice here. Also, I think you could just return NULL in this case. > + > + if (wide) > + MultiByteToWideChar(CP_ACP, 0, name, -1, (SEC_WCHAR*)buffer, SCHAN_KEY_NAME_MAX); > + else > + strcpy((SEC_CHAR*)buffer, name); > +} > + > static SECURITY_STATUS ensure_remote_cert(struct schan_context *ctx) > { > HCERTSTORE cert_store; > @@ -989,6 +1039,7 @@ static SECURITY_STATUS SEC_ENTRY schan_QueryContextAttributesW( > > if (!context_handle) return SEC_E_INVALID_HANDLE; > ctx = schan_get_object(context_handle->dwLower, SCHAN_HANDLE_CTX); > + if (!ctx) return SEC_E_INVALID_HANDLE; This has an effect outside of scope of this patch. This should be be a separated patch, with a test case. > switch(attribute) > { > @@ -1016,6 +1067,19 @@ static SECURITY_STATUS SEC_ENTRY schan_QueryContextAttributesW( > > return status; > } > + case SECPKG_ATTR_KEY_INFO: > + { > + SecPkgContext_KeyInfoW *info = buffer; > + SECURITY_STATUS status = schan_imp_get_key_info(ctx->session, info); > + if (status == SEC_E_OK) > + { > + info->sSignatureAlgorithmName = ctx->key_signature_name; > + info->sEncryptAlgorithmName = ctx->key_encrypt_name; > + fill_alg_name(info->SignatureAlgorithm, info->sSignatureAlgorithmName, TRUE); > + fill_alg_name(info->EncryptAlgorithm, info->sEncryptAlgorithmName, TRUE); > + } > + return status; > + } > case SECPKG_ATTR_REMOTE_CERT_CONTEXT: > { > PCCERT_CONTEXT *cert = buffer; > @@ -1091,6 +1155,24 @@ static SECURITY_STATUS SEC_ENTRY schan_QueryContextAttributesA( > { > case SECPKG_ATTR_STREAM_SIZES: > return schan_QueryContextAttributesW(context_handle, attribute, buffer); > + case SECPKG_ATTR_KEY_INFO: > + { > + SecPkgContext_KeyInfoA *info = buffer; > + struct schan_context *ctx; > + SECURITY_STATUS status; > + if (!context_handle) return SEC_E_INVALID_HANDLE; > + ctx = schan_get_object(context_handle->dwLower, SCHAN_HANDLE_CTX); > + if (!ctx) return SEC_E_INVALID_HANDLE; > + status = schan_imp_get_key_info(ctx->session, (SecPkgContext_KeyInfoW*)info); > + if (status == SEC_E_OK) > + { > + info->sSignatureAlgorithmName = (SEC_CHAR *)ctx->key_signature_name; > + info->sEncryptAlgorithmName = (SEC_CHAR *)ctx->key_encrypt_name; > + fill_alg_name(info->SignatureAlgorithm, info->sSignatureAlgorithmName, FALSE); > + fill_alg_name(info->EncryptAlgorithm, info->sEncryptAlgorithmName, FALSE); > + } > + return status; > + } > case SECPKG_ATTR_REMOTE_CERT_CONTEXT: > return schan_QueryContextAttributesW(context_handle, attribute, buffer); > case SECPKG_ATTR_CONNECTION_INFO: > diff --git a/dlls/secur32/schannel_gnutls.c b/dlls/secur32/schannel_gnutls.c > index 30640d3..ddf33a8 100644 > --- a/dlls/secur32/schannel_gnutls.c > +++ b/dlls/secur32/schannel_gnutls.c > @@ -363,6 +363,22 @@ static ALG_ID schannel_get_kx_algid(int kx) > } > } > > +static ALG_ID schannel_get_kx_signature_algid(gnutls_kx_algorithm_t kx) > +{ > + switch (kx) > + { > + case GNUTLS_KX_UNKNOWN: return 0; > + case GNUTLS_KX_RSA: > + case GNUTLS_KX_RSA_EXPORT: > + case GNUTLS_KX_DHE_RSA: > + case GNUTLS_KX_ECDHE_RSA: return CALG_RSA_SIGN; > + case GNUTLS_KX_ECDHE_ECDSA: return CALG_ECDSA; > + default: > + FIXME("unknown algorithm %d\n", kx); > + return 0; > + } > +} > + > unsigned int schan_imp_get_session_cipher_block_size(schan_imp_session session) > { > gnutls_session_t s = (gnutls_session_t)session; > @@ -394,6 +410,23 @@ SECURITY_STATUS schan_imp_get_connection_info(schan_imp_session session, > return SEC_E_OK; > } > > +SECURITY_STATUS schan_imp_get_key_info(schan_imp_session session, SecPkgContext_KeyInfoW *info) > +{ > + gnutls_session_t s = (gnutls_session_t)session; > + gnutls_cipher_algorithm_t alg = pgnutls_cipher_get(s); > + gnutls_kx_algorithm_t kx = pgnutls_kx_get(s); > + > + TRACE("(%p,%p)\n", session, info); > + > + info->sSignatureAlgorithmName = NULL; /* filled later */ > + info->sEncryptAlgorithmName = NULL; /* filled later */ > + info->KeySize = pgnutls_cipher_get_key_size(alg) * 8; > + info->SignatureAlgorithm = schannel_get_kx_signature_algid(kx); > + info->EncryptAlgorithm = schannel_get_cipher_algid(alg); > + > + return SEC_E_OK; > +} I don't think such helper that fills output partially is the right solution here. You could use schan_imp_get_connection_info to get all info you need except signature algorithm. For signature algorithm, you may introduce a new, simpler, function. Thanks, Jacek

1 0

Re: [5/6] secur32/tests: Add tests with SECPKG_ATTR_KEY_INFO.
by Jacek Caban April 19, 2017

April 19, 2017

Hi Akihiro, On 17.04.2017 16:26, Akihiro Sagawa wrote: > @@ -923,6 +924,15 @@ todo_wine > ok(conn_info.dwHashStrength >= 128, "conn_info.dwHashStrength = %d\n", conn_info.dwHashStrength); > } > > + status = pQueryContextAttributesA(&context, SECPKG_ATTR_KEY_INFO, &key_info); > + todo_wine_if(status == SEC_E_UNSUPPORTED_FUNCTION) > + ok(status == SEC_E_OK, "QueryContextAttributesW(SECPKG_ATTR_KEY_INFO) failed: %08x\n", status); I think this should use todo_wine, not todo_wine_if(). Thanks, Jacek

1 0

Re: [PATCH 01/10] wined3d: Introduce struct wined3d_dirty_regions.
by Stefan Dösinger April 19, 2017

April 19, 2017

Hi Masanori, Thank you for your patches! I have a few comments: Regarding the big picture I think it would be great if we had a scheme that also worked for partially mapping resources. It's also necessary to make the D3DLOCK_NO_DIRTY_UPDATE test on managed textures work. However, doing this is a lot more work than your patchset and complicates location management a lot. If Henri tells you something different follow his advice. He is the authority for decisions in d3d :-) . Patch 1: Am 2017-04-17 um 15:47 schrieb Masanori Kakura: > +struct wined3d_dirty_regions > +{ > + UINT left; > + UINT top; > + UINT right; > + UINT bottom; > + UINT front; > + UINT back; > + struct wined3d_dirty_regions *next; > +}; A standard wine list would be better IMHO. Patch 2: > diff --git a/dlls/wined3d/wined3d_private.h b/dlls/wined3d/wined3d_private.h > index 785105d7b9..711f98a0a6 100644 > --- a/dlls/wined3d/wined3d_private.h > +++ b/dlls/wined3d/wined3d_private.h > @@ -2765,6 +2765,27 @@ struct wined3d_dirty_regions > struct wined3d_dirty_regions *next; > }; > > +static inline BOOL wined3d_dirty_regions_add(struct wined3d_dirty_regions **regions, > + UINT left, UINT top, UINT right, UINT bottom, UINT front, UINT back) I don't think we want this in the header file, I think it'd be better placed in resource.c. Also note that buffers already have a related mechanism (buffer->modified_areas). You can predeclare the functions in the header if you need to call them from multiple files. > +{ > + struct wined3d_dirty_regions *new_region; > + > + if (!(new_region = HeapAlloc(GetProcessHeap(), 0, sizeof(struct wined3d_dirty_regions)))) > + return FALSE; HeapAlloc is expensive. I think it is OK for the start, but in the long run a way to keep some old dirty region elements around and reuse them instead of always calling Alloc / Free. Patch 5: > + if (!wined3d_dirty_regions_add(&resource->dirty_regions, 0, 0, width, height, 0, depth)) Why not use wined3d_resource_maximize_dirty_region here? > wined3d_resource_acquire(resource); > + wined3d_dirty_regions_clear(&resource->dirty_regions); > wined3d_cs_destroy_object(resource->device->cs, wined3d_resource_destroy_object, resource); Regions need to be cleared from inside the cs thread in case there are pending update_texture commands. Similarly patch 6 needs to put the region modifying into wined3d_cs_exec_add_dirty_texture_region. Patch 7: > + if (box->left < box->right && box->top < box->bottom && box->front < box->back) > + { You don't need to check the box, texture_resource_sub_resource_map already does it before it reaches this place of the code. See wined3d_texture_check_box_dimensions. Patch 8: I am not sure if this is necessary at all. In d3d9 it doesn't matter because a UpdateSurface destination can never be an UpdateTexture source. Patch 9/10: These should be merged, and I don't think patch 10 looks right. I think dirty regions should be honored on sublevels as well, just adjusted (i.e., divide by 2 for every level). > + if (src_texture->resource.width <= dst_texture->resource.width > + && src_texture->resource.height <= dst_texture->resource.height > + && src_texture->resource.depth <= dst_texture->resource.depth) Why are you adding this check? Is it because the src_skip_levels broke the if (i == 0) root level detection? If you apply the dirty region to all levels this problem should go away.

3 8

Re: [PATCH 6/6] dwrite: Make CreateFontFaceFromHdc() properly fail on unsupported font format
by Marvin April 19, 2017

April 19, 2017

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=30243 Your paranoid android. === wvistau64 (32 bit font) === font.c:3510: Test failed: got 0x80004005 === w2008s64 (32 bit font) === font.c:3510: Test failed: got 0x80004005 === wvistau64 (64 bit font) === font.c:3510: Test failed: got 0x80004005 === w2008s64 (64 bit font) === font.c:3510: Test failed: got 0x80004005

1 0

Re: [2/3] msi: Properly handle DWORD registry values in MsiGetPatchInfoEx.
by Marvin April 19, 2017

April 19, 2017

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=30239 Your paranoid android. === w7u (32 bit msi) === The task timed out

1 0