Re: [PATCH 1/3] dpwsockx: Stub implementation

[Stefan Dösinger]

Yay, Dplay work! A few suggestions though:

From patch 1:
> +static HRESULT WINAPI DPWSCB_GetCaps( LPDPSP_GETCAPSDATA data )
> +{
> - TRACE( "(%d,%p,0x%08x,%p)\n",
> -
>       data->idPlayer, data->lpCaps, data->dwFlags, data->lpISP );
> - return DP_OK;
>
> +}

Is there any reason this one writes a TRACE instread of a FIXME? I noticed 
that patch 3 implements it, perhaps you missed this when separating the 
patches?
> +static HRESULT WINAPI DPWSCB_Open( LPDPSP_OPENDATA data )
> +{
> - FIXME( "(%u,%p,%p,%u,0x%08x,0x%08x) stub\n",
> -
>       data->bCreate, data->lpSPMessageHeader, data->lpISP,
> -
>       data->bReturnStatus, data->dwOpenFlags, data->dwSessionFlags );
> - return DP_OK;
>
> +}

Why does it return DP_OK while most other stubs return an error?

Patch 2:
> - /* Initialize internal data */
> - ZeroMemory( &dpwsData, sizeof(DPWS_DATA) );

I think memset(&dpwsData, 0, sizeof(DPWS_DATA)) is preferred over ZeroMemory.

Patch 3:
Where do the values like dwMaxLocalPlayers = 65536 come from? Since most 
functions are still stubs its hard to see where it comes from? Does native 
dpwsockx have the same limits? (If it does, that's a solid reason for using 
the same limits)

[PATCH 1/3] dpwsockx: Stub implementation

[Ismael Barros²]

On Sat, Aug 29, 2009 at 12:25 PM, Stefan Dösinger wrote:
> Yay, Dplay work! A few suggestions though:
>
> From patch 1:
> > +static HRESULT WINAPI DPWSCB_GetCaps( LPDPSP_GETCAPSDATA data )
> > +{
> > - TRACE( "(%d,%p,0x%08x,%p)\n",
> > - data->idPlayer, data->lpCaps, data->dwFlags, data->lpISP );
> > - return DP_OK;
> >
> > +}
>
> Is there any reason this one writes a TRACE instread of a FIXME? I noticed
> that patch 3 implements it, perhaps you missed this when separating the
> patches?

Hum, true, I missed it, but anyway it gets implemented two patches later.
> > +static HRESULT WINAPI DPWSCB_Open( LPDPSP_OPENDATA data )
> > +{
> > - FIXME( "(%u,%p,%p,%u,0x%08x,0x%08x) stub\n",
> > - data->bCreate, data->lpSPMessageHeader, data->lpISP,
> > - data->bReturnStatus, data->dwOpenFlags, data->dwSessionFlags );
> > - return DP_OK;
> >
> > +}
>
> Why does it return DP_OK while most other stubs return an error?

I implemented it a year ago and I don't remember clearly that detail,
but it most probably was just to avoid dpwsoxks initialization failing
so I could keep fixing invalid parameter detection inside dplayx.
Anyway same issue than above, the function is implemented but it will
come in the next series of patches, when I finish reviewing them.
> Patch 2:
> > - /* Initialize internal data */
> > - ZeroMemory( &dpwsData, sizeof(DPWS_DATA) );
>
> I think memset(&dpwsData, 0, sizeof(DPWS_DATA)) is preferred over ZeroMemory.

I wasn't informed, but the codebase seems to confirm it:

[raziel@Bebop dlls]$ grep -Pr "memset([^,]+,[ ]*0" * | wc -l
    5565
    [raziel@Bebop dlls]$ grep -r ZeroMemory * | wc -l
    660

That should probably go into the doc
(http://www.winehq.org/site/developer-cheatsheet), like the advice of
using HeapAlloc instead of malloc.
> Patch 3:
> Where do the values like dwMaxLocalPlayers = 65536 come from? Since most
> functions are still stubs its hard to see where it comes from? Does native
> dpwsockx have the same limits? (If it does, that's a solid reason for using
> the same limits)

They come from the tests results, I copied them from the original
implementation. Some of them have a documented behaviour (like
substracting the size of the headers from the max buffer size), but
others seem to be arbitrary. They may need some tweaking depending on
the Windows version, but that will be shown over time by further
testing.

Thanks for the feedback :)