On 7/22/2015 8:11 PM, Niall Douglas wrote:
On 22 Jul 2015 at 13:35, Glen Fernandes wrote:
1. Use of documented kernel APIs: I'm torn here; on one hand, I don't condemn use of them and I would agree with Asbjørn that it isn't a show stopper. Yet you have a case of an actual show-stopper (crash) in AFIO because of the use of them.[1]
The show stopper crash is due to how ASIO implements its IOCP reactor which just happens to collide with the WOW64 bug. If I could modify the ASIO source code to special case this it's fixed. This is why Microsoft marked it as wonfix because even though it's a bug in their code, it is really super easy to work around if you modify your IOCP reactor.
As I mentioned before, if I don't hand off the async to ASIO I make the problem go away.
2. Use of undocumented or deprecated APIs: Kernel APIs or otherwise, even if apparently innocuous, this practice is alarming to me. I would be surprised if it doesn't taint the review of AFIO.
I can replace ObjectNameInformation with a new routine which uses non-deprecated APIs if the review demands it. Indeed, the ObjectNameInformation use replaced some original code which iterated every mounted volume in the system per path lookup, which was costly.
I suppose one _could_ make the assumption that open file handles can never be relocated across mounted volumes and that mounted volumes are never remounted anywhere new while file handles are open within them. With those two assumptions I can use NtQueryFileInformation instead, and reconstruct the same thing NtQueryObject returns.
What worried me was the possibility that a volume could be mounted in two or more locations you see, and that the canonical path of a file could change, hence the iteration of all mounted volumes to check for that.
I think this is the wrong question to ask. The right question to ask is: [snip]
So the wrong question to ask is, "Is it concerning that AFIO uses these APIs given your use of them is unsupported?" and the right question to ask is "Is it concerning that AFIO use these APIs given your use of them is unsupported but the advantages are [...]?" ? :-)
There is unsupported and there is unsupported. If you want to write a user mode program which executes in the Windows boot sequence before the subsystems start up (i.e. there is no win32 API available yet), then you write that program using the NT kernel API. That's supported, because it's categorised as a DDK user mode application.
By unsupported Microsoft have never said "don't use these APIs directly", they actually said "we won't promise we won't change anything inside the header file Winternl.h" and it just happens that the kernel API falls into that header file along with a ton of other stuff. I may have mentioned earlier that Cygwin and Mingw use the same APIs as AFIO, and if you review the Windows bug tracker where those projects report bugs in Windows you'll find a common pattern of the first tier support in Microsoft refusing the fix the bug as its use is "unsupported", then a more senior tier fixes the bug anyway.
That's because the single biggest user of the NT kernel API is Microsoft itself, and Microsoft would internally like bugs in one of its most fundamental APIs fixed, so they take these bug reports seriously.
If this isn't "supported", it looks awfully like supported. I would actually say the real situation is the Microsoft doesn't _encourage_ direct user mode use of NT kernel APIs. Historically speaking it wouldn't have worked on WinCE and Win95, and that was once a problem. I think we're a long way away from that now.
BTW the Visual Studio MSVCRT makes extensive use of NT kernel calls itself, indeed Stephan pointed me at RtlGenRandom which solved a problem for me.
Niall Douglas wrote:
I would say all three of these yes.
Sounds like a good enough spot to put yourself out of your misery and take this opportunity to escape the review queue that you've been in for so long. I understand wanting to improve the library, but something tells me you don't want to be in the queue for another two years constantly refactoring AFIO. Just my two cents.
I would certainly prefer review sooner rather than later. It could be if AFIO had been reviewed much earlier on I wouldn't need to do so much refactoring now, but maybe it's too niche for that. I don't know, I haven't even proved the case for futures instead of async_result yet, and I already know the ASIO crowd here is going to be united on that.
FWIW as a user I fully expect Boost to internally use undocumented APIs or even potentially unsafe code if necessary to make things smaller/faster/better, and handle all of the edge cases that make them dangerous and hide it all behind a nice interface. Basically exactly what AFIO is doing here. If what's in Boost isn't optimal then I'm usually going to have to consider rolling my own or using some other library which will be less tested, less documented, and less peer-reviewed.