Hi,
Note lots of <snip>s here to try and focus the discussion...
On 09/12/2012 02:37 AM, Pete Batard wrote:
> On 2012.09.11 09:25, Hans de Goede wrote:
<snip>
>> As said before, others have claim, others may get detach and I expect them
>> to hit the same race-like issues too once they get detach ...
>
> Then I guess the question becomes: should we add a new API even if we
> expect at least one for the platforms we support never to have any use
> for it?
Yes, I want libusbx to enable app writers to do whatever they envision
and their chosen platform makes possible, rather then offer some sort of
lowest common denominator approach. But I'm pretty sure we agree on that as
a goal, we just need to find common ground wrt the way to reach that goal :)
<snip>
>> What you're inventing here feels a lot like
>> the venerable unix ioctl(),
>
> Thanks! That's *exactly* what I've been going for. I should have
> realized it some time ago...
>
> Now that it is spelt out, I'm even more in favour of introducing a
> libusb_ioctl() call, with this exact name. It would make exceedingly
> explicit for our users to figure out what it's all about.
Hmm, actually having a libusb_ioctl could work out nicely for platform
specific stuff. It feels a bit low-level-ish, where as in general we
try to offer a higher-level API. But then again these bits tend to be
pretty low-level stuff.
<snip>
> I already mentioned the _EX/_V2 approach on Windows, which I have
> reservations about, and I would say that, as long as you're targeting C,
> you're going to be pretty limited... Doesn't mean that I'm not curious
> finding out how they skirt around good old ioctl, but if the idea is to
> just introduce plain API calls, I'd advise them to have a look at the
> evolution of MS APIs to get an idea of what it entails, and especially
> the infamous _EX_V2() creep.
Sometimes a V2 ioctl gets introduced under Linux. I like what the v4l2
Linux subsystem does there, as you probably know, the way Linux ioctls
work is they take:
Argument 1: a fd, would be a libusb_device_handle in our case
Argument 2: an int indicating which operation to do
Argument 3: a long giving the arguments to the operation, generally used to
pass a void *, so we should use a void *, as sizeof(long) != sizeof (void *)
on some non Linux platforms.
Since a lot of operations need more then one argument, there are structs
defined in various headers, and the past in argument actually is an address
of such a struct. This is where the better way to handle the v2 problem comes
in. what the v4l2 API does, is define structs like this:
struct ioctl_foo_data {
int foo;
int bar;
char reserved[56];
};
And specify in the documentation that the entire strucy should be memset to 0
before filling it. Then if later on an extra argument is needed, it gets defined
in such a way that the extra arg being 0 gives the old behavior, and the struct
gets changed to:
struct ioctl_foo_data {
int foo;
int bar;
int extra_extra_read_all_about_it;
char reserved[52];
};
Rince-repeat for v3, etc.
So we could add the following function to libusb:
int libusb_ioctl(libusb_device_handle *dev, int request, void *arguments);
And:
#define LIBUSB_IOCTL_DETACH_AND_CLAIM 1
And possibly:
struct libusb_ioctl_detach_and_claim {
const char *driver_name;
unsigned int interface;
char reserved[60 - sizeof(void *)];
};
Note that some tricks are needed when adding pointers,
to make sure that the struct size stays the same when
adding extra fields, ie adding an extra pointer now would make it:
struct libusb_ioctl_detach_and_claim {
const char *driver_name;
unsigned int interface;
char reserved[4];
const char *extra_pointer;
char reserved2[56 - 2 * sizeof(void *)];
};
Which is a pain, so usually in kernel ioctls pointers inside the
struct are avoided and instead one would do:
struct libusb_ioctl_detach_and_claim {
unsigned int interface;
char driver_name[256];
char reserved[60];
};
Then coding calling it would look like this (assuming c99) :
struct libusb_ioctl_detach_and_claim dc = {
.interface = 2,
.driver_name = "usb-mass-storage",
};
r = libusb_ioctl(handle, LIBUSB_IOCTL_DETACH_AND_CLAIM, &dc);
Versus the single line:
r = libusb_detach_kernel_driver_and_claim(handle, 2, "usb-mass-storage");
So the ioctl method requires slightly more lines, but I believe it is
acceptable.
<snip>
> Thus my proposal, with a single libusb_ioctl, is to make sure that we
> explicitly state that those OS IOCTLs are provided AS IS, with no
> guarantee of being carried in the same form, or at all, from one version
> of to the next, as well as ensuring that should we need to alter a call,
> we can do so without obvious breakage.
> IMO, this right to err or to alter is a lot more difficult to achieve
> with standalone and very public API calls.
Right, I'm fine with removing ioctl's at our leisure, and simply make
libusb_ioctl return LIBUSB_ERROR_NOT_SUPPORTED for unknown requests, but we
must make sure we never re-use request numbers during an ABI stable series.
IE, given the above example, if we ever drop LIBUSB_IOCTL_DETACH_AND_CLAIM,
we would keep the define in our list of defines (and drop the struct),
changing the define to:
#define LIBUSB_IOCTL_DEPRECATED1 1
The reason for keeping the defines around with a different name is to avoid
accidental re-use of the numbers.
<snip>
>> So counter proposal:
>> 1) We add something like the following to libusb.h:
>>
>> #define LIBUSB_PLATFORM_LINUX 1
>> #define LIBUSB_PLATFORM_WINDOWS 2
>> #define LIBUSB_PLATFORM_DARWIN 3
>> #define LIBUSB_PLATFORM_BSD 4
>>
>> #define LIBUSB_PLATFORM @LIBUSB_PLATFORM@
>>
>> And move libusb.h to libusb.h.in and let configure produce libusb.h during
>> the build filling in @LIBUSB_PLATFORM@
>
> So, now I need to figure out a libusb.h.in -> libusb.h for MSVC (DDK and
> VS2010 have no autotools)...
Ah right, so forget the configure thing, just a bunch of:
#if defined(__linux__)
#define LIBUSB_PLATFORM LIBUSB_PLATFORM_LINUX
#else if ....
to libusb.h, but if we go with the ioctl method this all becomes mute...
<snip>
>> We could even retro-actively apply this to existing platform specific
>> functions, at least at the documentation level, we should still build
>> in empty stubs returning LIBUSB_ERROR_NOT_SUPPORTED for ABI compatibility.
>
> Well, if we go with that I'd see an either^or there:
> Either we go with requiring #ifdefs and don't have to return
> NOT_SUPPORTED from the function only being listed in the API for the
> platforms it actually applies, or no #ifdef, and the app is required to
> handle NOT_SUPPORTED being returned.
Right, so the idea was to require apps to #ifdef for new platform specific
calls, like libusb_detach_kernel_driver_and_claim, and also at the
documentation and even in libusb.h level enforce that, but still keep
the few platform specific functions we already have in the .dll / .so
on platforms which don't have them for ABI compat, so basically go fully
for the #ifdef story, but keep ABI compat.
> Now, seeing that:
> - I wouldn't mind getting some idea where we want to go with regards to
> OS specific calls before we introduce the detach_and_claim()
> - I don't have a major issue introducing the current patch as proposed,
> since we already have detach for all platforms, and an exception to a
> future and currently imprecise rule for OS specific call isn't going to
> harm us much
> - I kind of agree that these would be nice to have in 1.0.13 rather than
> to 1.0.14
> - Time-wise, I could use some delaying of the RC and release by a few
> days (no more than a week though), especially as, as expected, some last
> minutes patches are being rushed in (and I'm also guilty as charged on that)
I propose that we first make a decision on how to move forward with
platform specific calls. If we go for libusb_ioctl, then I would like to
start with ioctls for:
kernel_driver_active
detach_kernel_driver
attach_kernel_driver
detach_kernel_driver_and_claim
As those seem to make more sense as ioctls then as the current functions we've
for the 1st three, the functions will stick around and just be wrappers
calling libusb_ioctl, and the functions will get marked as deprecated.
This however is too much work for 1.0.13 IMHO, as you've guessed I'm leaning
towards going with the ioctl implementation. In that case I vote for delaying
the libusb_detach_kernel_driver_and_claim patches to 1.0.14, and implement that
functionality through an ioctl right away.
If we decide to go the ioctl way I'll make some time to work on this in a couple
of weeks, or you can beat me to it :)
Regards,
Hans
------------------------------------------------------------------------------
Live Security Virtual Conference
Exclusive live event will cover all the ways today's security and
threat landscape has changed and how IT managers can respond. Discussions
will include endpoint security, mobile security and the latest in malware
threats. http://www.accelacomm.com/jaw/sfrnl04242012/114/50122263/
_______________________________________________
libusbx-devel mailing list
libusbx-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/libusbx-devel
