From: Dmitry F. <dm...@da...> - 2015-03-03 09:04:06
|
Hi Pete, Thanks for your reply. Please find my answers below. > On Mar 3, 2015, at 01:09 AM, Pete Batard <pe...@ak...> wrote: > > 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. While UsbDk doesn’t require installation per-device, device is “captured” automatically when client application (libusb or other) opens it via UsbDk_StartRedirect() API, it may have perfect sense to simulate installation by using “Hider API” to detach device from Windows OS. This is good to avoid Windows “New hardware found” pop-ups for devices that do not have “traditional” Windows drivers. See http://cgit.freedesktop.org/spice/win32/usbdk/tree/UsbDkHelper/UsbDkHelperHider.h <http://cgit.freedesktop.org/spice/win32/usbdk/tree/UsbDkHelper/UsbDkHelperHider.h> for hider API definitions. > > 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. This series introduce compile time selection of Windows backend. Indeed, it is easy to make this selection run-time, we even have patch for that to simplify our internal testing. In order to do this we need to define backend selection rules that will be good for everyone. There are 2 possible approaches: 1. Add explicit API to libusb for selection of the backend 2. Extend current libusb backend selection logic that selects libusbk/winusb sub-api based on libusbk is presence in the system. Overall I think run time selection mechanism is better than compile time and option 1 is better that option 2. What do you think? > > 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? Since UsbDk support is totally separate part of code there should be no changes in other sub-api’s behaviour. The only change in other sub-api’s code is moving some functions to separate file, so unless something was broken by patch 1 from the series, there should be no changes in other sub-api’s logic. > > 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. Good point! > > 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? libusb supports Windows CE, this is why we added “nt” to the prefix. > > 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”? The same as with function names, windows CE support is the problem. I think you’re right regarding this and “nt” should be changed to something newer. If Windows CE support is not an issues, I’d go for “windows_core" prefix. I tend to change it in next version of the series if there is no objections. > > > 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 Thanks again, Pete, ~Dmitry > 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 > > > ------------------------------------------------------------------------------ > Dive into the World of Parallel Programming The Go Parallel Website, sponsored > by Intel and developed in partnership with Slashdot Media, is your hub for all > things parallel software development, from weekly thought leadership blogs to > news, videos, case studies, tutorials and more. Take a look and join the > conversation now. http://goparallel.sourceforge.net/ > _______________________________________________ > libusb-devel mailing list > lib...@li... > https://lists.sourceforge.net/lists/listinfo/libusb-devel |