[PATCH 3/8] [v2] wsdapi: Add support for sending messages via UDP multicast.

[Owen Rudge]

I've replaced the string address conversion with static numeric 
addresses, as suggested.

I have kept the send_message algorithm as it was before as it more 
closely reflects the proscribed algorithm in the SOAP spec, and the 
delay calculations do differ between the two. I think trying to alter it 
to reduce the number of lines would make the algorithm less clear.

Signed-off-by: Owen Rudge 
---
  dlls/wsdapi/Makefile.in       |   3 +-
  dlls/wsdapi/discovery.c       |   5 +
  dlls/wsdapi/network.c         | 252 
++++++++++++++++++++++++++++++++++++++++++
  dlls/wsdapi/soap.c            |  17 ++-
  dlls/wsdapi/tests/discovery.c |   4 +-
  dlls/wsdapi/wsdapi_internal.h |   6 +
  6 files changed, 282 insertions(+), 5 deletions(-)
  create mode 100644 dlls/wsdapi/network.c

[Huw Davies]

On Thu, Mar 15, 2018 at 09:58:09PM +0000, Owen Rudge wrote:
> I've replaced the string address conversion with static numeric addresses,
> as suggested.
>
> I have kept the send_message algorithm as it was before as it more closely
> reflects the proscribed algorithm in the SOAP spec, and the delay
> calculations do differ between the two. I think trying to alter it to reduce
> the number of lines would make the algorithm less clear.

There are still some very long lines in this patch (and others in the
series).  Personally, I find anything over c.120 hard to follow.
> +BOOL send_udp_multicast_of_type(char *data, int length, int max_initial_delay, ULONG family)
> +{
> - IP_ADAPTER_ADDRESSES *adapter_addresses = NULL, *adapter_addr;
> - sending_thread_params *send_params;
> - ULONG bufferSize = 0;
> - const struct in6_addr i_addr_zero = {0};

Any reason why you didn't use static const here?  Then you don't need the
explicit initialization, which clang wants to be {{0}} anyway.
> - LPSOCKADDR sockaddr;
> - BOOL ret = FALSE;
> - HANDLE thread_handle;
> - const char ttl = 8;
> - ULONG retval;
> - SOCKET s;
> -
> - /* Get size of buffer for adapters */
> - retval = GetAdaptersAddresses(family, 0, NULL, NULL, &bufferSize);
> -
> - if (retval != ERROR_BUFFER_OVERFLOW)
> - {
> -
>    WARN("GetAdaptorsAddresses failed with error %08x\n", retval);
> -
>    goto cleanup;
> - }
> -
> - adapter_addresses = (IP_ADAPTER_ADDRESSES *) heap_alloc(bufferSize);
> -
> - if (adapter_addresses == NULL)
> - {
> -
>    WARN("Out of memory allocating space for adapter information\n");
> -
>    goto cleanup;
> - }
> -
> - /* Get list of adapters */
> - retval = GetAdaptersAddresses(family, 0, NULL, adapter_addresses, &bufferSize);
> -
> - if (retval != ERROR_SUCCESS)
> - {
> -
>    WARN("GetAdaptorsAddresses failed with error %08x\n", retval);
> -
>    goto cleanup;
> - }
> -
> - for (adapter_addr = adapter_addresses; adapter_addr != NULL; adapter_addr = adapter_addr->Next)
> - {
> -
>    if (adapter_addr->FirstUnicastAddress == NULL)
> -
>    {
> -
>        TRACE("No address found for adaptor '%s' (%p)\n", debugstr_a(adapter_addr->AdapterName), adapter_addr);
> -
>        continue;
> -
>    }
> -
> -
>    sockaddr = adapter_addr->FirstUnicastAddress->Address.lpSockaddr;
> -
> -
>    /* Create a socket and bind to the adapter address */
> -
>    s = socket(family, SOCK_DGRAM, IPPROTO_UDP);
> -
> -
>    if (s == INVALID_SOCKET)
> -
>    {
> -
>        WARN("Unable to create socket: %d\n", WSAGetLastError());
> -
>        continue;
> -
>    }
> -
> -
>    if (bind(s, sockaddr, adapter_addr->FirstUnicastAddress->Address.iSockaddrLength) == SOCKET_ERROR)
> -
>    {
> -
>        WARN("Unable to bind to socket (adaptor '%s' (%p)): %d\n", debugstr_a(adapter_addr->AdapterName), adapter_addr, WSAGetLastError());
> -
>        closesocket(s);
> -
>        continue;
> -
>    }
> -
> -
>    /* Set the multicast interface and TTL value */
> -
>    setsockopt(s, IPPROTO_IP, IP_MULTICAST_IF, (char *) &i_addr_zero, (family == AF_INET6) ? sizeof(struct in6_addr) : sizeof(struct in_addr));
> -
>    setsockopt(s, IPPROTO_IP, IP_MULTICAST_TTL, &ttl, sizeof(ttl));
> -
> -
>    /* Set up the thread parameters */
> -
>    send_params = heap_alloc(sizeof(*send_params));
> -
> -
>    send_params->data = heap_alloc(length);
> -
>    memcpy(send_params->data, data, length);
> -
>    send_params->length = length;
> -
>    send_params->sock = s;
> -
>    send_params->max_initial_delay = max_initial_delay;
> -
> -
>    memset(&send_params->dest, 0, sizeof(SOCKADDR_STORAGE));
> -
>    send_params->dest.ss_family = family;
> -
> -
>    if (family == AF_INET)
> -
>    {
> -
>        SOCKADDR_IN *sockaddr4 = (SOCKADDR_IN *)&send_params->dest;
> -
> -
>        sockaddr4->sin_port = htons(SEND_PORT);
> -
>        sockaddr4->sin_addr.S_un.S_addr = htonl(SEND_ADDRESS_IPV4);
> -
>    }
> -
>    else
> -
>    {
> -
>        SOCKADDR_IN6 *sockaddr6 = (SOCKADDR_IN6 *)&send_params->dest;
> -
> -
>        sockaddr6->sin6_port = htons(SEND_PORT);
> -
>        memcpy(&sockaddr6->sin6_addr, &send_address_ipv6, sizeof(send_address_ipv6));
> -
>    }
> -
> -
>    thread_handle = CreateThread(NULL, 0, sending_thread, send_params, 0, NULL);

I wonder whether we really want a new thread / message.  This should probably be moved
to a thread pool api, but perhaps that can happen later on.
> -
> -
>    if (thread_handle == NULL)
> -
>    {
> -
>        WARN("CreateThread failed (error %d)\n", GetLastError());
> -
>        closesocket(s);
> -
> -
>        heap_free(send_params->data);
> -
>        heap_free(send_params);
> -
> -
>        continue;
> -
>    }
> -
> -
>    CloseHandle(thread_handle);
> - }
> -
> - ret = TRUE;
> -
>
> +cleanup:
> - if (adapter_addresses != NULL) heap_free(adapter_addresses);
> -
> - return ret;
>
> +}
> -
>
> +BOOL send_udp_multicast(IWSDiscoveryPublisherImpl *impl, char *data, int length, int max_initial_delay)
> +{
> - if ((impl->addressFamily & WSDAPI_ADDRESSFAMILY_IPV4) && (!send_udp_multicast_of_type(data, length, max_initial_delay, AF_INET))) return FALSE;
> - if ((impl->addressFamily & WSDAPI_ADDRESSFAMILY_IPV6) && (!send_udp_multicast_of_type(data, length, max_initial_delay, AF_INET6))) return FALSE;
> - return TRUE;
>
> +}
> -
>
> +void terminate_networking(IWSDiscoveryPublisherImpl *impl)
> +{
> - BOOL needsCleanup = impl->publisherStarted;
> -
> - impl->publisherStarted = FALSE;
> -
> - if (needsCleanup)
> -
>    WSACleanup();
>
> +}
> -
>
> +BOOL init_networking(IWSDiscoveryPublisherImpl *impl)
> +{
> - WSADATA wsaData;
> - int ret = WSAStartup(MAKEWORD(2, 2), &wsaData);
> -
> - if (ret != 0)
> - {
> -
>    WARN("WSAStartup failed with error: %d\n", ret);
> -
>    return FALSE;
> - }
> -
> - impl->publisherStarted = TRUE;
> -
> - /* TODO: Start listening */
> - return TRUE;
>
> +}
> diff --git a/dlls/wsdapi/soap.c b/dlls/wsdapi/soap.c
> index efc063ca05..bd73c73acb 100644
> --- a/dlls/wsdapi/soap.c
> +++ b/dlls/wsdapi/soap.c
> @@ -27,12 +27,15 @@
>  #include "wine/debug.h"
>  #include "wine/heap.h"
>
> +WINE_DEFAULT_DEBUG_CHANNEL(wsdapi);
> -
>
> #define APP_MAX_DELAY                       500
>
> static HRESULT write_and_send_message(IWSDiscoveryPublisherImpl *impl, WSD_SOAP_HEADER *header, WSDXML_ELEMENT *body_element,
>      struct list *discovered_namespaces, IWSDUdpAddress *remote_address, int max_initial_delay)
>  {
>      static const char xml_header[] = "<?xml version=\"1.0\" encoding=\"utf-8\"?>";
> -
>
> HRESULT ret = E_FAIL;
> char *full_xml;
>
> /* TODO: Create SOAP envelope */
>
> @@ -46,11 +49,21 @@ static HRESULT write_and_send_message(IWSDiscoveryPublisherImpl *impl, WSD_SOAP_
>      memcpy(full_xml, xml_header, sizeof(xml_header));
>      full_xml[sizeof(xml_header)] = 0;
> - /* TODO: Send the message */
> -
>
> if (remote_address == NULL)
> -
>
> {
> -
>    /* Send the message via UDP multicast */
> -
>    if (send_udp_multicast(impl, full_xml, sizeof(xml_header), max_initial_delay))
> -
>        ret = S_OK;
> -
>
> }
> -
>
> else
> -
>
> {
> -
>    /* TODO: Send the message via UDP unicast */
> -
>    FIXME("TODO: Send the message via UDP unicast\n");
> -
>
> }
>
> heap_free(full_xml);
> - return S_OK;
> - return ret;
>
> }
>
> HRESULT send_hello_message(IWSDiscoveryPublisherImpl *impl, LPCWSTR id, ULONGLONG metadata_ver, ULONGLONG instance_id,
> diff --git a/dlls/wsdapi/tests/discovery.c b/dlls/wsdapi/tests/discovery.c
> index e4a672b311..d81dcdc936 100644
> --- a/dlls/wsdapi/tests/discovery.c
> +++ b/dlls/wsdapi/tests/discovery.c
> @@ -570,7 +570,7 @@ static void Publish_tests(void)
>  /* Publish the service */
>  rc = IWSDiscoveryPublisher_Publish(publisher, publisherIdW, 1, 1, 1, NULL, NULL, NULL, NULL);
> - todo_wine ok(rc == S_OK, "Publish failed: %08x\n", rc);
> -
>
> ok(rc == S_OK, "Publish failed: %08x\n", rc);
>
> /* Wait up to 2 seconds for messages to be received */
> if (WaitForMultipleObjects(msgStorage->numThreadHandles, msgStorage->threadHandles, TRUE, 2000) == WAIT_TIMEOUT)
>
> @@ -583,7 +583,7 @@ static void Publish_tests(void)
>      DeleteCriticalSection(&msgStorage->criticalSection);
>  /* Verify we've received a message */
> - todo_wine ok(msgStorage->messageCount >= 1, "No messages received\n");
> -
>
> ok(msgStorage->messageCount >= 1, "No messages received\n");
>
> sprintf(endpointReferenceString, "wsa:EndpointReferencewsa:Address%s</wsa:Address></wsa:EndpointReference>", publisherId);
>
> diff --git a/dlls/wsdapi/wsdapi_internal.h b/dlls/wsdapi/wsdapi_internal.h
> index 631dfb3d7e..417eed9145 100644
> --- a/dlls/wsdapi/wsdapi_internal.h
> +++ b/dlls/wsdapi/wsdapi_internal.h
> @@ -49,6 +49,12 @@ typedef struct IWSDiscoveryPublisherImpl {
>      BOOL                  publisherStarted;
>  } IWSDiscoveryPublisherImpl;
>
> +/* network.c */
> -
>
> +BOOL init_networking(IWSDiscoveryPublisherImpl *impl);
> +void terminate_networking(IWSDiscoveryPublisherImpl *impl);
> +BOOL send_udp_multicast(IWSDiscoveryPublisherImpl *impl, char *data, int length, int max_initial_delay);
> -
>
> /* soap.c */
>
> HRESULT send_hello_message(IWSDiscoveryPublisherImpl *impl, LPCWSTR id, ULONGLONG metadata_ver, ULONGLONG instance_id,
>

[Owen Rudge]

Hi Huw,
> There are still some very long lines in this patch (and others in the
> series).  Personally, I find anything over c.120 hard to follow.

Sorry, I think I had amended line length in some of the earlier patches then forgot with the later patches - will check those again.
> Any reason why you didn't use static const here?  Then you don't need the
> explicit initialization, which clang wants to be {{0}} anyway.

Thanks, I'll amend that.
> I wonder whether we really want a new thread / message.  This should probably be moved
> to a thread pool api, but perhaps that can happen later on.

Mainly because on systems with a several network interfaces, this can result in the function call taking a long time, due to the delays enforced by the WSD/SOAP specifications. Sending messages simultaneously on several threads speeds this up considerably. On Windows the API appears to return near-instantly so I presume it's doing the same.

A thread pool would likely be sensible but I'd prefer not to rewrite this code again at present if possible.

Thanks,

Owen

[Huw Davies]

On Mon, Mar 19, 2018 at 11:11:11AM +0000, Owen Rudge wrote:
> > I wonder whether we really want a new thread / message.  This should probably be moved
> > to a thread pool api, but perhaps that can happen later on.
>
> Mainly because on systems with a several network interfaces, this
> can result in the function call taking a long time, due to the
> delays enforced by the WSD/SOAP specifications. Sending messages
> simultaneously on several threads speeds this up considerably. On
> Windows the API appears to return near-instantly so I presume it's
> doing the same.

I realise it needs to be done in a background thread.  I was
commenting that creating a *new* thread for each message seems
expensive, but this can probably wait for later.

Huw.

[Owen Rudge]

Hi Huw,
> I realise it needs to be done in a background thread.  I was
> commenting that creating a *new* thread for each message seems
> expensive, but this can probably wait for later.

Ah, sorry, I misunderstood that. Yes, a thread pool would be better in the long run. Publish tends to be called once at the start of an application or service, so it's less critical in terms of performance.

Thanks,

Owen

[Huw Davies]

On Mon, Mar 19, 2018 at 11:39:28AM +0000, Owen Rudge wrote:
> Hi Huw,
> > I realise it needs to be done in a background thread.  I was
> > commenting that creating a *new* thread for each message seems
> > expensive, but this can probably wait for later.
>
> Ah, sorry, I misunderstood that. Yes, a thread pool would be better
> in the long run. Publish tends to be called once at the start of an
> application or service, so it's less critical in terms of
> performance.

Ok, if it typically only gets called once, this is less of an issue.

Thanks,

Huw.