From: Pete B. <pe...@ak...> - 2015-03-02 23:09:13
|
Hi Dmirty. First of all, thanks for submitting these patches. UsbDk looks quite interesting, and something we should try to have support for in libusb. *If* that makes sense (which is something I'm not entirely sure for now) I may very well try to add UsbDk support into libwdi/Zadig [1], as the goal of libwdi is to facilitate installation for any of the drivers libusb supports, and there already is some limited support for libusb0 as a filter driver there. But for the time being, I have to stress out that I only had a very quick glance at what you submitted, and the following remarks are only meant to scratch the surface. On 2015.03.02 17:59, Dmitry Fleytman wrote: > Switch between UsbDk and WinUSB/LibusbK/Libusb0 is done at compile time > with --enable-usbdk configuration option (off by default). Does "switch between" mean that you can only have one or the other? Looking at the configure.ac changes, it looks like it is meant to coexist with the other Windows sub-APIs, so I'm a bit confused with your use of "switch" above, in case it means users will have to choose one or the other. Also, in case UsbDk can indeed coexist with the other Windows sub APIs, I personally don't like the idea of disabling it by default, even if it's new untested code, as this is bound to introduce confusion with regards to the library capabilities when we deal with our users. Besides potential bugs that introduction of new code might introduce (and that we can work through) do you have concerns that, in its current state, UsbDk support is likely to break support for the other Windows sub APIs? Also, you may want to bear in mind that the number of libusb testers on Windows is more limited than you might think, so if UsbDk is introduced as an optional feature, you're probably not going to get as much testing of your code as you'd like. IMO, better make the feature non optional, and sort anything we need to sort as it pops up. From [PATCH 1/3]: > libusb/os/windows_nt_common.c | 572 > libusb/os/windows_nt_common.h | 67 (...) > + void win_nt_destroy_clock() (...) > + int win_nt_handle_events(struct libusb_context *ctx, struct pollfd *fds, POLL_NFDS_TYPE nfds, int num_ready) I'm gonna have a very *subjective* objection to suffixing anything with "NT" in 2015! The use of NT was already obsolete in 2000, so, _please_, let's not go for monikers that have long ran their course when introducing new files and function calls. And yes, I know that NT is still being used by Windows and Microsoft internally, but that's only because of legacy... which we're absolutely not being bound to here. Or do we anticipate that we'll have to support a different Windows architecture than NT in the future? The reason I'm not too happy with that suffix is, if I was new to libusb, and started looking at the Windows source, anything suffixed in _nt would make me think that I'm dealing with code that probably hasn't been updated in more than 15 years, regardless of its content... and thus wouldn't inspire me to want to do anything with it. Besides, libusb has some history of using the designation "core" for common stuff, so, to bring a little bit of harmonization between what Windows and the rest of libusb does, I'd prefer to see the use of win_core_function_name() as a more modern alternative. Then again, if anybody has other suggestions, as long as they're not using terminology that's rooted in the past, I'm open to them. Also, what was wrong with just "windows_common.c" and "windows_common.h"? That's all I have to superficially comment on for now. I'll try to have a closer look if/when I get a chance, but then again, I'm quite happy to see that Chris already has plans to do so in a much shorter timeframe (and I agree with him that this is something that we want to introduce before 1.0.21). Regards, /Pete [1] https://github.com/pbatard/libwdi/wiki |