dpnet patches

[Stefan Dösinger]

Hi Alistair,

A while ago I promised to have a closer look at your dpnet patches. It’s a bit delayed, but here are some comments. How serious are you about dplay? Do you intend to do more work towards implementing it, or is that just wishful thinking on my part?

http://source.winehq.org/patches/data/101857:
The entire patch doesn’t do all that much, I think it’s best to hold of on that change until the function is meaningfully implemented - unless having a proper return value helps you in some way I’m missing.
> -
>        void *buffer = malloc(256);

Please use HeapAlloc / HeapFree.

http://source.winehq.org/patches/data/101856:
> +DEFINE_GUID(GUID_NULL,0,0,0,0,0,0,0,0,0,0,0);

Isn’t that defined somewhere already? Do you need GUID_NULL to signal an uninitialized GUID? Is there a reason why setting the pointer to NULL doesn’t work?
> -IMPORTS   = dpnet ole32
> +IMPORTS   = ole32

I’m not sure about this one, but winetest can automatically skip tests if the tested DLL doesn’t exist. It might use the imported libraries for that, or maybe just the name of the test. In the former case I think you want to keep the dpnet import.

http://source.winehq.org/patches/data/101855:
http://source.winehq.org/patches/data/101854:
Did you think about sharing code between IDirectPlay8Peer and IDirectPlay8Server? The interfaces themselves have no relationship, but their methods are similar.

http://source.winehq.org/patches/data/101853:
> +typedef struct IDirectPlay8ClientImpl
> +{
> - IDirectPlay8Client IDirectPlay8Client_iface;
> - LONG ref;
>
> +} IDirectPlay8ClientImpl;

Are you sure you want to move the definition out of the library-wide header? If that works out ok it’s great, but you may need something other than the public interface in some other files in this library.

I recommend to work without the typedef, but that should be changed in a separate patch.
> - ULONG refCount = InterlockedIncrement(&This->ref);
> - ULONG ref = InterlockedIncrement(&This->ref);
> - TRACE("(%p)->(ref before=%u)\n", This, refCount - 1);
> - TRACE("(%p) ref=%d\n", This, ref);

ULONG is unsigned, you have to use %u.

http://source.winehq.org/patches/data/101852
> -
>                'DPNSPModemModem'
> -
>                {
> -
>                    val 'Friendly Name' = s 'Modem Connection For DirectPlay'
> -
>                    val 'GUID' = s '{6D4A3650-628D-11D2-AE0F-006097B01411}'
> -
>                }
> -
> -
>                'DPNSPModemSerial'
> -
>                {
> -
>                    val 'Friendly Name' = s 'Serial Connection For DirectPlay'
> -
>                    val 'GUID' = s '{743B5D60-628D-11D2-AE0F-006097B01411}'
> -
>                }

I know that those always exist on Windows, but wouldn’t it be better to not advertise them until we have an actual implementation? I think advertising DPNSPWinsockTCP alone should be ok for the start.

[Alistair Leslie-Hughes]

Hi Stefan,

On 31/01/2014 11:39 PM, Stefan Dösinger wrote:
> A while ago I promised to have a closer look at your dpnet patches. It’s a bit delayed, but here are some comments. How serious are you about dplay? Do you intend to do more work towards implementing it, or is that just wishful thinking on my part?

Thanks for you comments. I do plan on implementing as much as I can, but 
with limited time it's going to happen over a weekend.
> > -IMPORTS   = dpnet ole32
> > +IMPORTS   = ole32
>
> I’m not sure about this one, but winetest can automatically skip tests if the tested DLL doesn’t exist. It might use the imported libraries for that, or maybe just the name of the test. In the former case I think you want to keep the dpnet import.
>
> http://source.winehq.org/patches/data/101855:
> http://source.winehq.org/patches/data/101854:
> Did you think about sharing code between IDirectPlay8Peer and IDirectPlay8Server? The interfaces themselves have no relationship, but their methods are similar.

Yes, I did think that these two interfaces would need to share a lot of 
code.
> http://source.winehq.org/patches/data/101853:
> > +typedef struct IDirectPlay8ClientImpl
> > +{
> > - IDirectPlay8Client IDirectPlay8Client_iface;
> > - LONG ref;
> >
> > +} IDirectPlay8ClientImpl;
>
> Are you sure you want to move the definition out of the library-wide header? If that works out ok it’s great, but you may need something other than the public interface in some other files in this library.

Sure,  I leave it where it for now.
> I recommend to work without the typedef, but that should be changed in a separate patch.
> > - ULONG refCount = InterlockedIncrement(&This->ref);
> > - ULONG ref = InterlockedIncrement(&This->ref);
> > - TRACE("(%p)->(ref before=%u)\n", This, refCount - 1);
> > - TRACE("(%p) ref=%d\n", This, ref);
>
> ULONG is unsigned, you have to use %u.

I'll fix that.
> I know that those always exist on Windows, but wouldn’t it be better to not advertise them until we have an actual implementation? I think advertising DPNSPWinsockTCP alone should be ok for the start.

Agreed, I'll adjust the register file.

Thanks for all you comments.

Alistair.