Bug 63807

This is fixed by this commit, closing:
http://cgit.freedesktop.org/spice/spice-gtk/commit/?id=0ebcc23c0642d2bddcb05ed4152497c99cb3ca1d

(In reply to comment #1)
> This is fixed by this commit, closing:
> http://cgit.freedesktop.org/spice/spice-gtk/commit/
> ?id=0ebcc23c0642d2bddcb05ed4152497c99cb3ca1d

Sorry but you are not getting away that easy. :) As I pointed out on IRC, its a very ugly API if you are asking apps to give you a very cryptic string for filter. Surely you can add some enums/flags and allow apps to filter through that. If you fail in that, at the very least add some docs explaining the syntax of the filter string you are expecting from apps.

Also, I think the name is long enough so I suggest renaming this function to: pice_usb_device_manager_get_devices_filtered() but this is just a suggestion. Ignore if you don't agree. :)

Hi Zeeshan,

The format string for the filter is the same as in RHEV-M USB filter editor tool, and it is actually documented; the filtering is provided is part of usbredirparser library, e.g. http://cgit.freedesktop.org/~jwrdegoede/usbredir/tree/usbredirparser/usbredirfilter.h. 

Short description an example of that string is provided in e.g. gtk/usb-device-manager.c, 

   /**
     * SpiceUsbDeviceManager:auto-connect-filter:
     *
     * Set a string specifying a filter to use to determine which USB devices
     * to autoconnect when plugged in, a filter consists of one or more rules.
     * Where each rule has the form of:
     *
     * @class,@vendor,@product,@version,@allow
     *
     * Use -1 for @class/@vendor/@product/@version to accept any value.
     *
     * And the rules are themselves are concatonated like this:
     *
     * @rule1|@rule2|@rule3
     *
     * The default setting filters out HID (class 0x03) USB devices from auto
     * connect and auto connects anything else. Note the explicit allow rule at
     * the end, this is necessary since by default all devices without a
     * matching filter rule will not auto-connect.
     *
     * Filter strings in this format can be easily created with the RHEV-M
     * USB filter editor tool.
     */
    pspec = g_param_spec_string("auto-connect-filter", "Auto Connect Filter ",
               "Filter determining which USB devices to auto connect",
               "0x03,-1,-1,-1,0|-1,-1,-1,-1,1",
               G_PARAM_READWRITE | G_PARAM_CONSTRUCT | G_PARAM_STATIC_STRINGS);

I agree that having a string-based API is sub-optimal. I'm not good in library API nor in GLib/GTK though - but guess it is written in C on purpose. 

I can think of exposing an API which accepts something like an array of rules (from usbredirfilter.h) instead of the filter string:

struct usbredirfilter_rule {
    int device_class;       /* 0-255, -1 to match any class */
    int vendor_id;          /* 0-65535, -1 to match any id */
    int product_id;         /* 0-65535, -1 to match any id */
    int device_version_bcd; /* 0-255, -1 to match any version */
    int allow;              /* 0: deny redir for this device, non 0: allow */
};

const presets for filtering e.g. HIDs may be added easily:
const struct usbredirfilter_rule USBREDIRFILTER_RULE_DENY_HID =
{
    0x03,
    -1,
    -1,
    -1,
    0
}

Guess this isn't really convenient as well. Ok, what we need to filter usually? Let's think that we already have a low-level API that may be inconvenient for users. OK, users need to filter by device class. So we can add a convenience API:

GPtrArray* spice_usb_device_manager_get_devices_filtered_by_class(
SpiceUsbDeviceManager *self, const GArray *usb_device_classes_to_exclude);

Then to get the filtered list of devices you'd be able to write something like:

GArray * usb_device_classes_to_exclude = g_array_new(false, false, sizeof(guint));
g_array_append_val(usb_device_classes_to_exclude, USB_CLASS_HID);
g_array_append_val(usb_device_classes_to_exclude, USB_CLASS_AUDIO);
spice_usb_device_manager_get_devices_filtered_by_class(usb_device_manager, usb_device_classes_to_exclude);

We can define these USB_CLASS_xxxxx as described at http://www.usb.org/developers/defined_class: 
const guint USB_CLASS_AUDIO 0x01
...
const guint USB_CLASS_HID 0x03

Highly probably they're defined somewhere in Spice or e.g. libusb already, so maybe we should reuse them from there.

Will this convenience function satisfy you? 

Hans, is it feasible to add something like this to SpiceUsbDeviceManager? I can work on this.. I need a hint on what container to use (i.e. is GArray a right choice here?)

(In reply to comment #2)
> (In reply to comment #1)
> > This is fixed by this commit, closing:
> > http://cgit.freedesktop.org/spice/spice-gtk/commit/
> > ?id=0ebcc23c0642d2bddcb05ed4152497c99cb3ca1d
> 
> Sorry but you are not getting away that easy. :) As I pointed out on IRC,
> its a very ugly API if you are asking apps to give you a very cryptic string
> for filter. Surely you can add some enums/flags and allow apps to filter
> through that. If you fail in that, at the very least add some docs
> explaining the syntax of the filter string you are expecting from apps.

Zeeshan, please, take a look on:
http://lists.freedesktop.org/archives/spice-devel/2014-July/017092.html

(In reply to comment #4)
> (In reply to comment #2)
> > (In reply to comment #1)
> > > This is fixed by this commit, closing:
> > > http://cgit.freedesktop.org/spice/spice-gtk/commit/
> > > ?id=0ebcc23c0642d2bddcb05ed4152497c99cb3ca1d
> > 
> > Sorry but you are not getting away that easy. :) As I pointed out on IRC,
> > its a very ugly API if you are asking apps to give you a very cryptic string
> > for filter. Surely you can add some enums/flags and allow apps to filter
> > through that. If you fail in that, at the very least add some docs
> > explaining the syntax of the filter string you are expecting from apps.
> 
> Zeeshan, please, take a look on:
> http://lists.freedesktop.org/archives/spice-devel/2014-July/017092.html

Looks good to me.

(In reply to comment #2)
> (In reply to comment #1)
> > This is fixed by this commit, closing:
> > http://cgit.freedesktop.org/spice/spice-gtk/commit/
> > ?id=0ebcc23c0642d2bddcb05ed4152497c99cb3ca1d
> 
> Sorry but you are not getting away that easy. :) As I pointed out on IRC,
> its a very ugly API if you are asking apps to give you a very cryptic string
> for filter. Surely you can add some enums/flags and allow apps to filter
> through that. If you fail in that, at the very least add some docs
> explaining the syntax of the filter string you are expecting from apps.

I have to disagree, a query string is actually more friendly API here.


However, I agree with you the API could be better documented (move the filter description to the API section & add links).

(In reply to comment #6)
> (In reply to comment #2)
> > (In reply to comment #1)
> > > This is fixed by this commit, closing:
> > > http://cgit.freedesktop.org/spice/spice-gtk/commit/
> > > ?id=0ebcc23c0642d2bddcb05ed4152497c99cb3ca1d
> > 
> > Sorry but you are not getting away that easy. :) As I pointed out on IRC,
> > its a very ugly API if you are asking apps to give you a very cryptic string
> > for filter. Surely you can add some enums/flags and allow apps to filter
> > through that. If you fail in that, at the very least add some docs
> > explaining the syntax of the filter string you are expecting from apps.
> 
> I have to disagree, a query string is actually more friendly API here.

You'll have to do better than just saying that. :) Most APIs can be "easy" with strings but more static APIs are always preferred for a few reasons. Firstly, you get errors from compiler if you screw up with static API. Secondly, with C many times developers just reference API from headers rather than docs if the API is obvious (which we should try our best to do).

Also, its not nice that you have ignored this up til now when someone decided to fix the issue. :(

(In reply to comment #6)
> However, I agree with you the API could be better documented (move the
> filter description to the API section & add links).

I disagree. documentation has a reference to the part of the code that already describes the rule and filter format. I'm closing the bug as WONTFIX.

(In reply to comment #8)
> (In reply to comment #6)
> > However, I agree with you the API could be better documented (move the
> > filter description to the API section & add links).
> 
> I disagree. documentation has a reference to the part of the code that
> already describes the rule and filter format. I'm closing the bug as WONTFIX.

How is disagreeing with this lead to WONTFIX all of a sudden? Did you read my reply above at all?

(In reply to comment #9)
> (In reply to comment #8)
> > (In reply to comment #6)
> > > However, I agree with you the API could be better documented (move the
> > > filter description to the API section & add links).
> > 
> > I disagree. documentation has a reference to the part of the code that
> > already describes the rule and filter format. I'm closing the bug as WONTFIX.
> 
> How is disagreeing with this lead to WONTFIX all of a sudden? Did you read
> my reply above at all?

Yeah, I did. As commented in the patch Hans also disagrees with this approach.
That's the reason I've closed it as WONTFIX.

(In reply to comment #10)
> (In reply to comment #9)
> > (In reply to comment #8)
> > > (In reply to comment #6)
> > > > However, I agree with you the API could be better documented (move the
> > > > filter description to the API section & add links).
> > > 
> > > I disagree. documentation has a reference to the part of the code that
> > > already describes the rule and filter format. I'm closing the bug as WONTFIX.
> > 
> > How is disagreeing with this lead to WONTFIX all of a sudden? Did you read
> > my reply above at all?
> 
> Yeah, I did. As commented in the patch Hans also disagrees with this
> approach.

Hans has always disagreed so that is not news. Pity you realized that after putting up a patch in the right direction and getting my hopes up. :(

> That's the reason I've closed it as WONTFIX.

Would have been nicer if Hans had updated this bug one year ago so I wouldn't have hoped spice team is capable of comping up with good APIs.

BTW, even with the current-string based API the correct resolution is FIXED, not WONTFIX.

(In reply to comment #11)
> (In reply to comment #10)
> > (In reply to comment #9)
> > > (In reply to comment #8)
> > > > (In reply to comment #6)
> > > > > However, I agree with you the API could be better documented (move the
> > > > > filter description to the API section & add links).
> > > > 
> > > > I disagree. documentation has a reference to the part of the code that
> > > > already describes the rule and filter format. I'm closing the bug as WONTFIX.
> > > 
> > > How is disagreeing with this lead to WONTFIX all of a sudden? Did you read
> > > my reply above at all?
> > 
> > Yeah, I did. As commented in the patch Hans also disagrees with this
> > approach.
> 
> Hans has always disagreed so that is not news. Pity you realized that after
> putting up a patch in the right direction and getting my hopes up. :(
> 
> > That's the reason I've closed it as WONTFIX.
> 
> Would have been nicer if Hans had updated this bug one year ago so I
> wouldn't have hoped spice team is capable of comping up with good APIs.
> 
> BTW, even with the current-string based API the correct resolution is FIXED,
> not WONTFIX.

I have the patch half-way done, as you could see.
Instead of a list of structures we could have a list of device codes (int) and that would be the cleaner solution I could come up with and still looks like it is not that helpful from the SPICE point of view, so there is not much I can do here.

What I'm going to do is reopen this bug and wait for Christophe's opinion, as he was one the people interested in a better API.

(In reply to comment #12)> 
> What I'm going to do is reopen this bug and wait for Christophe's opinion,
> as he was one the people interested in a better API.

And here is a link for the v2:
http://lists.freedesktop.org/archives/spice-devel/2014-July/017092.html

(In reply to comment #5)
> (In reply to comment #4)
> > (In reply to comment #2)
> > > (In reply to comment #1)
> > > > This is fixed by this commit, closing:
> > > > http://cgit.freedesktop.org/spice/spice-gtk/commit/
> > > > ?id=0ebcc23c0642d2bddcb05ed4152497c99cb3ca1d
> > > 
> > > Sorry but you are not getting away that easy. :) As I pointed out on IRC,
> > > its a very ugly API if you are asking apps to give you a very cryptic string
> > > for filter. Surely you can add some enums/flags and allow apps to filter
> > > through that. If you fail in that, at the very least add some docs
> > > explaining the syntax of the filter string you are expecting from apps.

Sure, but a string is a *lot* more flexible, friendly for humans, and you can still get errors if it is malformed. I don't get your coment about API, it's the string format that is used by spice-gtk for USB filtering, it's documented and we are not going to duplicate this everywhere as it works perfectly so far.

You can implement a utility function in your code trivially, so please consider this FIXED. (I'd even say that it's not spice-gtk job to enumerate system USB devices, it's nice that Hans added one already)

(In reply to comment #14)
> Sure, but a string is [...] friendly for humans

I'll have to disagree with at least that bit ;)
A human can build a valid string if they have the documentation handy, this does not mean this is a friendly way of filtering USB devices.

(In reply to comment #15)
> (In reply to comment #14)
> > Sure, but a string is [...] friendly for humans
> 
> I'll have to disagree with at least that bit ;)
> A human can build a valid string if they have the documentation handy, this
> does not mean this is a friendly way of filtering USB devices.

It's friendly because that's what human type and read. That allows to have simple user command line arguments, easy and compact variable and such.

Closing for the reasons mentionned earlier.

(In reply to comment #14)
> (In reply to comment #5)
> > (In reply to comment #4)
> > > (In reply to comment #2)
> > > > (In reply to comment #1)
> > > > > This is fixed by this commit, closing:
> > > > > http://cgit.freedesktop.org/spice/spice-gtk/commit/
> > > > > ?id=0ebcc23c0642d2bddcb05ed4152497c99cb3ca1d
> > > > 
> > > > Sorry but you are not getting away that easy. :) As I pointed out on IRC,
> > > > its a very ugly API if you are asking apps to give you a very cryptic string
> > > > for filter. Surely you can add some enums/flags and allow apps to filter
> > > > through that. If you fail in that, at the very least add some docs
> > > > explaining the syntax of the filter string you are expecting from apps.
> 
> Sure, but a string is a *lot* more flexible, friendly for humans, and you
> can still get errors if it is malformed.

At runtime, yes. What if you had to change string format tomorrow? How would you resolve the issue of existing binaries of Boxes (and other using apps) breaking against it?

Also, IDEs won't help you with autocompletion with such dynamic strings either, which they would if you provide a proper static API. I am very surprised with your assertions here since I know that you are a great developer with lots of experience. :(

oh and btw the actual bug really if fixed, no matter how much I hate the solution. :)

(In reply to comment #17)
> (In reply to comment #14)
> > (In reply to comment #5)
> > > (In reply to comment #4)
> > > > (In reply to comment #2)
> > > > > (In reply to comment #1)
> > > > > > This is fixed by this commit, closing:
> > > > > > http://cgit.freedesktop.org/spice/spice-gtk/commit/
> > > > > > ?id=0ebcc23c0642d2bddcb05ed4152497c99cb3ca1d
> > > > > 
> > > > > Sorry but you are not getting away that easy. :) As I pointed out on IRC,
> > > > > its a very ugly API if you are asking apps to give you a very cryptic string
> > > > > for filter. Surely you can add some enums/flags and allow apps to filter
> > > > > through that. If you fail in that, at the very least add some docs
> > > > > explaining the syntax of the filter string you are expecting from apps.
> > 
> > Sure, but a string is a *lot* more flexible, friendly for humans, and you
> > can still get errors if it is malformed.
> 
> At runtime, yes. What if you had to change string format tomorrow? How would
> you resolve the issue of existing binaries of Boxes (and other using apps)
> breaking against it?

Just like printf? it won't break.

> Also, IDEs won't help you with autocompletion with such dynamic strings
> either, which they would if you provide a proper static API. I am very
> surprised with your assertions here since I know that you are a great
> developer with lots of experience. :(

I already explained myself, there are tradeoffs here. If you want a stricter API, you can implement it on top.