On Mon, 2013-05-06 at 19:23 -0600, Erich E. Hoover wrote:
--- a/dlls/iphlpapi/iphlpapi_main.c +++ b/dlls/iphlpapi/iphlpapi_main.c @@ -210,8 +210,14 @@ DWORD WINAPI AllocateAndGetIpAddrTableFromStack(PMIB_IPADDRTABLE *ppIpAddrTable, */ BOOL WINAPI CancelIPChangeNotify(LPOVERLAPPED overlapped) {
- FIXME("(overlapped %p): stub\n", overlapped);
- return FALSE;
- HANDLE handle;
- TRACE("(overlapped %p)\n", overlapped);
- if (!overlapped)
return FALSE;- handle = (HANDLE) overlapped->u.Pointer;
- return TerminateThread(handle, STATUS_CANCELLED);
}
TerminateThread is not the right tool here. If the thread is blocked on the recv call while being terminated it will leak the netlink socket.
You'd need a way to synchronize the threads, but an overhead of one thread per caller is rather costly in the first place. This suggests that the server should poll the socket.
On Tue, May 7, 2013 at 3:14 AM, Hans Leidekker hans@codeweavers.com wrote:
... TerminateThread is not the right tool here. If the thread is blocked on the recv call while being terminated it will leak the netlink socket.
You'd need a way to synchronize the threads, but an overhead of one thread per caller is rather costly in the first place. This suggests that the server should poll the socket.
So far the iphlpapi hasn't been doing anything directly with the server... How about launching a single thread for NotifyAddrChange (when needed) and keeping a list of OVERLAPPED structures to signal when the list changes? It's easy enough to use MSG_DONTWAIT to make recv non-blocking so that it's easy to cancel the thread.
Best, Erich
On Tue, 2013-05-07 at 10:04 -0600, Erich E. Hoover wrote:
On Tue, May 7, 2013 at 3:14 AM, Hans Leidekker hans@codeweavers.com wrote:
... TerminateThread is not the right tool here. If the thread is blocked on the recv call while being terminated it will leak the netlink socket.
You'd need a way to synchronize the threads, but an overhead of one thread per caller is rather costly in the first place. This suggests that the server should poll the socket.
So far the iphlpapi hasn't been doing anything directly with the server... How about launching a single thread for NotifyAddrChange (when needed) and keeping a list of OVERLAPPED structures to signal
It's a system-wide event so the server seems to be a pretty good candidate to handle it, and it wouldn't need any extra threads.
when the list changes? It's easy enough to use MSG_DONTWAIT to make recv non-blocking so that it's easy to cancel the thread.
You'd be consuming cpu instead of properly waiting. I guess you could make this work if you poll an array with the netlink fd and another unix fd to signal the cancel event, but again, I think this is best handled by the server.
On Tue, May 7, 2013 at 12:26 PM, Hans Leidekker hans@codeweavers.com wrote:
... It's a system-wide event so the server seems to be a pretty good candidate to handle it, and it wouldn't need any extra threads. ...
Ok, how about something like the attached? (I assume the first patch is OK)
Best, Erich
On Wed, 2013-05-08 at 15:34 -0600, Erich E. Hoover wrote:
+#ifdef HAVE_LINUX_NETLINK_H +# include <linux/netlink.h> +#endif +#ifdef HAVE_LINUX_RTNETLINK_H +# include <linux/rtnetlink.h> +#endif
+#ifdef NLMSG_OK +# define WINE_LINKMON_FAMILY PF_NETLINK +# define WINE_LINKMON_TYPE SOCK_RAW +# define WINE_LINKMON_PROTO NETLINK_ROUTE +# define WINE_LINKMON_ADDRCHANGE(b) (NLMSG_OK((struct nlmsghdr*)b, len) \
&& (((struct nlmsghdr*)b)->nlmsg_type == RTM_NEWADDR \|| ((struct nlmsghdr*)b)->nlmsg_type == RTM_DELADDR))+#endif
I don't see the need for these defines. WINE_LINKMON_ADDRCHANGE could be turned into a function.
@@ -2043,12 +2100,79 @@ DWORD WINAPI IpRenewAddress(PIP_ADAPTER_INDEX_MAP AdapterInfo)
- FIXME
- Stub, returns ERROR_NOT_SUPPORTED.
*/ -DWORD WINAPI NotifyAddrChange(PHANDLE Handle, LPOVERLAPPED overlapped) +DWORD WINAPI NotifyAddrChange(PHANDLE handle, LPOVERLAPPED overlapped) {
- FIXME("(Handle %p, overlapped %p): stub\n", Handle, overlapped);
- if (Handle) *Handle = INVALID_HANDLE_VALUE;
- if (overlapped) ((IO_STATUS_BLOCK *) overlapped)->u.Status = STATUS_PENDING;
- return ERROR_IO_PENDING;
+#ifdef WINE_LINKMON_FAMILY
- IO_STATUS_BLOCK *iosb = (IO_STATUS_BLOCK *) overlapped;
- struct sockaddr_nl addr;
- NTSTATUS status;
- int fd = -1;
- HANDLE h;
- TRACE("(handle %p, overlapped %p): stub\n", handle, overlapped);
- h = INVALID_HANDLE_VALUE;
- SERVER_START_REQ( create_socket )
Using a generic socket object might work I guess. The cost will be a new socket per caller whereas in theory we could serve all callers with a single socket, though that would require a dedicated server request. That might be needed anyway, if this handle somehow turns out to be special.
I know MacOS has a similar socket based mechanism that could probably be handled in the same way.
On Fri, May 10, 2013 at 3:48 AM, Hans Leidekker hans@codeweavers.com wrote:
On Wed, 2013-05-08 at 15:34 -0600, Erich E. Hoover wrote:
... +#ifdef NLMSG_OK +# define WINE_LINKMON_FAMILY PF_NETLINK +# define WINE_LINKMON_TYPE SOCK_RAW +# define WINE_LINKMON_PROTO NETLINK_ROUTE +# define WINE_LINKMON_ADDRCHANGE(b) (NLMSG_OK((struct nlmsghdr*)b, len) \
&& (((struct nlmsghdr*)b)->nlmsg_type == RTM_NEWADDR \|| ((struct nlmsghdr*)b)->nlmsg_type == RTM_DELADDR))+#endif
I don't see the need for these defines. WINE_LINKMON_ADDRCHANGE could be turned into a function.
For the reason you noted below, on Mac it would make sense to set these defines to be PF_SYSTEM, SOCK_RAW, and SYSPROTO_EVENT (respectively). I wasn't sure whether the address change would be better as a define or a function so I made it a define to fit with the others, but I'd be happy to change it if you think it'd be better as a function. It also seems that on Mac one uses an ioctl() request instead of bind() to finish setting up the socket, but I figured that having that as another define (or function) would be a little bit much for this patch.
...
- SERVER_START_REQ( create_socket )
Using a generic socket object might work I guess. The cost will be a new socket per caller whereas in theory we could serve all callers with a single socket, though that would require a dedicated server request. That might be needed anyway, if this handle somehow turns out to be special.
According to the documentation, the handle is soley for use in GetOverlappedResult - so I think we should be ok with implementing this with a generic socket for now. I don't think it's worth the effort to make a special server request just so that we can use a single socket for this call, as it's only useful for each application to open one of these handles (while you could create more than one they will _all_ wakeup when the address list changes). However, we could always change things later if we discover that the handle has special properties or if there's some application that has a lot of these handles floating around.
I know MacOS has a similar socket based mechanism that could probably be handled in the same way.
Just let me know what you think, thanks for taking the time to look this over!
Best, Erich
On Fri, 2013-05-10 at 08:00 -0600, Erich E. Hoover wrote:
On Fri, May 10, 2013 at 3:48 AM, Hans Leidekker hans@codeweavers.com wrote:
On Wed, 2013-05-08 at 15:34 -0600, Erich E. Hoover wrote:
... +#ifdef NLMSG_OK +# define WINE_LINKMON_FAMILY PF_NETLINK +# define WINE_LINKMON_TYPE SOCK_RAW +# define WINE_LINKMON_PROTO NETLINK_ROUTE +# define WINE_LINKMON_ADDRCHANGE(b) (NLMSG_OK((struct nlmsghdr*)b, len) \
&& (((struct nlmsghdr*)b)->nlmsg_type == RTM_NEWADDR \|| ((struct nlmsghdr*)b)->nlmsg_type == RTM_DELADDR))+#endif
I don't see the need for these defines. WINE_LINKMON_ADDRCHANGE could be turned into a function.
For the reason you noted below, on Mac it would make sense to set these defines to be PF_SYSTEM, SOCK_RAW, and SYSPROTO_EVENT (respectively). I wasn't sure whether the address change would be better as a define or a function so I made it a define to fit with the others, but I'd be happy to change it if you think it'd be better as a function. It also seems that on Mac one uses an ioctl() request instead of bind() to finish setting up the socket, but I figured that having that as another define (or function) would be a little bit much for this patch.
I think it would be nicer to push the platform specifics to helper functions (e.g. setting up the socket or checking for an address change) and use the usual header defines (HAVE_LINUX_NETLINK_H and/or HAVE_SYS_KERN_EVENT_H) for anything that needs to be compiled conditionally.
On Fri, May 10, 2013 at 8:27 AM, Hans Leidekker hans@codeweavers.com wrote:
... I think it would be nicer to push the platform specifics to helper functions (e.g. setting up the socket or checking for an address change) and use the usual header defines (HAVE_LINUX_NETLINK_H and/or HAVE_SYS_KERN_EVENT_H) for anything that needs to be compiled conditionally.
This look a little more like what you're looking for?
Best, Erich
On Fri, 2013-05-10 at 20:07 -0600, Erich E. Hoover wrote:
+/***********************************************************************
IPHLPAPI_createmonitorhandle (INTERNAL)
- Routine to create the interface monitoring handle for NotifyAddrChange requests.
- */
+static NTSTATUS IPHLPAPI_createmonitorhandle(HANDLE *h) +{
- NTSTATUS status = ERROR_NOT_SUPPORTED;
- *h = INVALID_HANDLE_VALUE;
+#if defined(NETLINK_ROUTE)
I don't see why you're still using these defines. It's unlikely that this code will work if NETLINK_ROUTE is defined but RTMGRP_IPV4_IFADDR isn't, for example.
If there are versions of the netlink headers that don't have all of them it may be possible to work around that (e.g. by defining them ourselves), if not we can expand the configure check.
On Mon, May 13, 2013 at 3:45 AM, Hans Leidekker hans@codeweavers.com wrote:
...
I don't see why you're still using these defines. It's unlikely that this code will work if NETLINK_ROUTE is defined but RTMGRP_IPV4_IFADDR isn't, for example.
If there are versions of the netlink headers that don't have all of them it may be possible to work around that (e.g. by defining them ourselves), if not we can expand the configure check.
I was just trying to use the defines that were in each section of code, since that's generally what I've seen in other places in Wine. I'm happy to standardize on NETLINK_ROUTE though, as I'm unaware of any issues with these features being unavailable in different versions of the headers. As soon as I get a chance I'll update that and resubmit, thanks for all your help!
Best, Erich
-
Erich E. Hoover -
Erich E. Hoover -
Hans Leidekker