Re: mmdevapi/tests: test MMDevPropStore_GetAt

[Marvin]

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://newtestbot.winehq.org/JobDetails.pl?Key=2403

Your paranoid android.


=== wvista (32 bit propstore) ===
propstore.c:108: Test failed: DEVPKEY_Device_FriendlyName not found

Re: [3/3] ws2_32: Implement WSASendMsg() (try 2)

[Alexandre Julliard]

Bruno Jesus <00cp...@gmail.com> writes:

> @@ -2427,6 +2427,46 @@ static void WINAPI WS2_GetAcceptExSockaddrs(PVOID 
> buffer, DWORD data_size, DWORD
>  }
>  
>  /***
> + * WSASendMsg
> + */
> +int WINAPI WSASendMsg( SOCKET s, LPWSAMSG msg, DWORD dwFlags, LPDWORD 
> lpNumberOfBytesSent,
> +   LPWSAOVERLAPPED lpOverlapped,
> +   LPWSAOVERLAPPED_COMPLETION_ROUTINE 
> lpCompletionRoutine)
> +{
> +int ret = SOCKET_ERROR, fd, sock_type;
> +socklen_t optlen = sizeof(sock_type);
> +
> +if (!msg)
> +{
> +SetLastError(WSAEFAULT);
> +return SOCKET_ERROR;
> +}
> +
> +if ( (fd = get_sock_fd( s, 0, NULL )) == -1)
> +return SOCKET_ERROR;
> +
> +if (getsockopt(fd, SOL_SOCKET, SO_TYPE, &sock_type, &optlen) == -1)
> +{
> +SetLastError((errno == EBADF) ? WSAENOTSOCK : wsaErrno());
> +goto cleanup;
> +}
> +
> +if (sock_type != SOCK_DGRAM && sock_type != SOCK_RAW)
> +{
> +SetLastError(WSAEINVAL);
> +goto cleanup;
> +}
> +
> +ret = WS2_sendto( s, msg->lpBuffers, msg->dwBufferCount, 
> lpNumberOfBytesSent,
> + dwFlags, msg->name, msg->namelen,
> + lpOverlapped, lpCompletionRoutine );

This is redundant, WS2_sendto will already fetch the file descriptor,
any necessary error checking should happen there.

-- 
Alexandre Julliard
julli...@winehq.org

Re: [PATCH 1/2] setupapi: Also look in datadir/fakedlls for the re-compiled fakedlls.

[Alexandre Julliard]

Huw Davies  writes:

> ---
>  dlls/setupapi/fakedll.c | 13 -
>  1 file changed, 12 insertions(+), 1 deletion(-)

I don't think that's correct. The datadir is for platform-independent
files, but the fakedlls are platform specific.

-- 
Alexandre Julliard
julli...@winehq.org

Re: atl90: Add new DLL

[Zhenbo Li]

2013/9/4 Zhenbo Li :
> This patch could fix bug 34431, which is caused by missing atl90.dll
>
> ---
>  configure  |  1 +
>  configure.ac   |  1 +
>  dlls/atl90/Makefile.in |  7 
>  dlls/atl90/atl90.c | 93
> ++
>  dlls/atl90/atl90.spec  | 52 
>  include/atlbase.h  |  1 +
>  tools/make_specfiles   |  1 +
>  7 files changed, 156 insertions(+)

Excuse me, this is my first time to submit a patch to Wine. And I
don't know that if I had made some mistakes. Would someone please
review my patch and tell me what to do?
Thank you.

-- 
Have a nice day!
Zhenbo Li

Increasing max_req_size and subsequent errors

[Hugh McMaster]

I need to increase the $max_req_size in tools/make_requests.  A new request 
structure for GetConsoleScreenBufferInfoEx is 80 bytes and primarily made up of 
16 unsigned long integers.  This is fine, but raises the error: 

wineserver: request.c:755: open_master_socket: Assertion `sizeof(union 
generic_reply) == sizeof(struct request_max_size)' failed.

It looks like this error is connected to

struct request_max_size
{
int pad[16];
};

in include/wine/server_protocol.h.  But other than increasing the size of the 
array, I'm not sure how to fix this appropriately.

Any advice will be appreciated.

Re: [2/3] kernel32/tests: test CopyFileEx callback and cancellation (resend)

[Juan Lang]

On Sun, Sep 29, 2013 at 11:10 PM, Daniel Jeliński wrote:

> 2013/9/30 Nikolay Sivov 
>
>> On 9/30/2013 00:51, Daniel Jeliński wrote:
>>
>>
>>  +struct progress_list {
>>> +const DWORD progress_retval_init;  /* value to return from progress
>>> routine */
>>> +const BOOL cancel_init;/* value to set Cancel flag to */
>>> +const DWORD progress_retval_end;   /* value to return from progress
>>> routine */
>>> +const BOOL cancel_end; /* value to set Cancel flag to */
>>> +const DWORD progress_count;/* number of times progress is
>>> invoked */
>>> +const BOOL copy_retval;/* expected CopyFileEx result */
>>> +const DWORD lastError; /* expected CopyFileEx error
>>> code */
>>> +} ;
>>>
>>  I don't see a point making them 'const'.
>>
> I'm matching the formatting of existing code:
> http://source.winehq.org/source/dlls/kernel32/tests/file.c#L65
> Also, what's the point of not making them const?
>

It's a little strange, stylewise. More consistent with C++ style would be
to make the entire struct constant. But in a test, we often eschew with
such things if they distract, and here I think they do.


>  +static DWORD WINAPI progress(LARGE_INTEGER TotalFileSize,
>>> +LARGE_INTEGER TotalBytesTransferred,
>>> +LARGE_INTEGER StreamSize,
>>> +LARGE_INTEGER StreamBytesTransferred,
>>> +DWORD dwStreamNumber,
>>> +DWORD dwCallbackReason,
>>> +HANDLE hSourceFile,
>>> +HANDLE hDestinationFile,
>>> +LPVOID lpData)
>>> +{
>>> +progressInvoked++;
>>>
>> Please pass all globals as context data with lpData, and please use
>> 'void*' instead of LPVOID.
>>
> Good point about lpData. Still, does that make the patch invalid? Why
> didn't you mention that on the first review?
> About LPVOID - I'm matching the headers:
> http://source.winehq.org/source/include/winbase.h#L910
> for the third patch:
> http://source.winehq.org/source/include/winbase.h#L1018
>

LPVOID is just an uglier #define of void*. It's easier to read as void*, so
please use that instead.
--Juan

Re: [2/3] kernel32/tests: test CopyFileEx callback and cancellation (resend)

[Nikolay Sivov]

On 9/30/2013 10:10, Daniel Jeliński wrote:
2013/9/30 Nikolay Sivov >


On 9/30/2013 00:51, Daniel Jeliński wrote:


+struct progress_list {
+const DWORD progress_retval_init;  /* value to return
from progress routine */
+const BOOL cancel_init;/* value to set Cancel
flag to */
+const DWORD progress_retval_end;   /* value to return
from progress routine */
+const BOOL cancel_end; /* value to set Cancel
flag to */
+const DWORD progress_count;/* number of times
progress is invoked */
+const BOOL copy_retval;/* expected CopyFileEx
result */
+const DWORD lastError; /* expected CopyFileEx
error code */
+} ;

 I don't see a point making them 'const'.

I'm matching the formatting of existing code:
http://source.winehq.org/source/dlls/kernel32/tests/file.c#L65
Also, what's the point of not making them const?

+static DWORD WINAPI progress(LARGE_INTEGER TotalFileSize,
+LARGE_INTEGER TotalBytesTransferred,
+LARGE_INTEGER StreamSize,
+LARGE_INTEGER StreamBytesTransferred,
+DWORD dwStreamNumber,
+DWORD dwCallbackReason,
+HANDLE hSourceFile,
+HANDLE hDestinationFile,
+LPVOID lpData)
+{
+progressInvoked++;

Please pass all globals as context data with lpData, and please
use 'void*' instead of LPVOID.

Good point about lpData. Still, does that make the patch invalid?
It's not black or white, I mentioned what will be nice to do about it to 
make it more compact and self-contained.
It doesn't mean it's invalid, it's just an obvious thing that will make 
it better.

Why didn't you mention that on the first review?

Because sometimes I stop reading further after I see major problems.



Also, any comments on patch 3?

Same thing, you could easily pack everything to a single struct and pass 
it using this context pointer.
It could also be made more compact using a single helper to compar 
COPYFILE2_MESSAGE value,

instead of duplicating it for every message type.

Re: [3/3] ws2_32: Implement WSASendMsg() (try 2)

[Bruno Jesus]

On Mon, Sep 30, 2013 at 6:32 AM, Alexandre Julliard  wrote:
>
> Bruno Jesus <00cp...@gmail.com> writes:
>
> > @@ -2427,6 +2427,46 @@ static void WINAPI WS2_GetAcceptExSockaddrs(PVOID 
> > buffer, DWORD data_size, DWORD
> >  }
> >
> >  /***
> > + * WSASendMsg
> > + */
> > +int WINAPI WSASendMsg( SOCKET s, LPWSAMSG msg, DWORD dwFlags, LPDWORD 
> > lpNumberOfBytesSent,
> > +   LPWSAOVERLAPPED lpOverlapped,
> > +   LPWSAOVERLAPPED_COMPLETION_ROUTINE 
> > lpCompletionRoutine)
>
> This is redundant, WS2_sendto will already fetch the file descriptor,
> any necessary error checking should happen there.

Ok, thanks for the review. I'll try again asap.

> --
> Alexandre Julliard
> julli...@winehq.org

Bruno

Re: [4/5] ntdll: Make asynchronous NtWriteFile on a disk file always return STATUS_PENDING.

[Alexandre Julliard]

Dmitry Timoshkov  writes:

> ---
>  dlls/ntdll/file.c   | 3 ++-
>  dlls/ntdll/tests/file.c | 5 +++--
>  2 files changed, 5 insertions(+), 3 deletions(-)

I don't see the point. Do you actually have an app that depends on this?

-- 
Alexandre Julliard
julli...@winehq.org

Re: wldap32: Use BOOL type where appropriate

[Rico Schüller]

On 01.10.2013 00:06, Frédéric Delanoy wrote:

---
  dlls/wldap32/init.c | 6 +++---
  1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/dlls/wldap32/init.c b/dlls/wldap32/init.c
index b5aed58..54b5b24 100644
--- a/dlls/wldap32/init.c
+++ b/dlls/wldap32/init.c
@@ -113,13 +113,13 @@ oom:
  }

  /* Determine if a URL starts with a known LDAP scheme */
-static int has_ldap_scheme( char *url )
+static BOOL has_ldap_scheme( char *url )
  {
  if (!strncasecmp( url, "ldap://";, 7 ) ||
  !strncasecmp( url, "ldaps://", 8 ) ||
  !strncasecmp( url, "ldapi://", 8 ) ||
-!strncasecmp( url, "cldap://";, 8 )) return 1;
-return 0;
+!strncasecmp( url, "cldap://";, 8 )) return TRUE;
+return FALSE;
  }

  /* Flatten an array of hostnames into a space separated string of URLs.

You probably may return the "value" of the if statement directly instead 
of returning TRUE / FALSE.


Cheers
Rico

Re: [1/3] gdi32: Return fake BBox when requested empty glyph metrics. (try 3)

[Bruno Jesus]

On Fri, Sep 27, 2013 at 8:16 AM, Akihiro Sagawa  wrote:
>
> Fixes bug 18440, 20847.
>
> Try 3:
>- Rebase to recent source.
>- Separate ABC metrics change.
>- Use conditional operator.
> ---
>  dlls/gdi32/freetype.c   |4 ++--
>  dlls/gdi32/tests/font.c |   24 
>  2 files changed, 10 insertions(+), 18 deletions(-)

> +ok(gm.gmBlackBoxY == 1, "%2d:expected 1, got %u\n", fmt[i], 
> gm.gmBlackBoxX);

There seems to be a copy&paste issue in all lines related to
gmBlackBoxY reporting the error as gmBlackBoxX.

Regards,
Bruno

Re: [4/5] ntdll: Make asynchronous NtWriteFile on a disk file always return STATUS_PENDING.

[Dmitry Timoshkov]

Alexandre Julliard  wrote:

> I don't see the point. Do you actually have an app that depends on this?

When I started to work on this I had an app that has one of the reasons to
require running under Vista+ was the difference in overlapped IO behavior.
On the other hand the tests clearly show that up-to-date Windows versions
(including Windows XP) always return STATUS_PENDING for asynchronous read
and write on disk files. Is there another way to remove all those todo_wine
statements from the tests?

-- 
Dmitry.

RE: Increasing max_req_size and subsequent errors

[Hugh McMaster]

Solved. server/protocol.def is the correct place to adjust the request_max_size 
value.