GNOME Bugzilla – Bug 698430
Don't allow user to redirect mouse & keyboard
Last modified: 2016-03-31 13:22:00 UTC
When one activate the USB redirection, and enable redirection for USB Mouse, the pointer is totally locked out in the host, and it's not possible to move it.
(In reply to comment #0) > When one activate the USB redirection, and enable redirection for USB Mouse, > the pointer is totally locked out in the host, and it's not possible to move > it. Firstly, its completely expected behavior that device being redirected is not available in host. i-e you can have a particular device either in host or guest, not both. However, we should not be exposing mouse and keyboard devices for USB redirection in Boxes. For doing so, we'll need new spice-gtk API: https://bugs.freedesktop.org/show_bug.cgi?id=63807 Anyway, don't think we can solve this in 3.8.
*** Bug 729034 has been marked as a duplicate of this bug. ***
spice-gtk has a way to do that now ( spice_usb_device_manager_get_devices_with_filter ) even if the API could be better.
Based on http://www.usb.org/developers/defined_class which are the classes we have interested to maintain? Everyone but HID (Human Interface Device)?
Created attachment 290252 [details] [review] Don't expose keyboards and mice for redirection Create a function that allows us to create a filter (in the format documented in usbredir[0]) and filter out the HID Class, responsible for keyboard, mice and joysticks. The decision to filter out the whole class was taken because there is no way to do it only keyboards and mice. [0]: http://cgit.freedesktop.org/spice/usbredir/tree/usbredirparser/usbredirfilter.h
Review of attachment 290252 [details] [review]: Kudos for documentation, in the log and in the comment! However: :) * Shorter shortlog please: Abbreviate 'keyboard' as 'kbd' and use '&' instead of 'and'. * "and filter out" -> "and use that to filter out". However it sounds like the function is generic and the caller is deciding to add the 'no HID' part to it, which isn't true. * " responsible for keyboard, mice and joysticks" -> " (keyboard, mice and joysticks)". * What about tablets? Are they included? * "no way to do it only keyboards and mice" -> "no way to filter out only keyboard and mouse". ::: src/spice-display.vala @@ +18,3 @@ private BoxConfig.SyncProperty[] gtk_session_sync_properties; private bool closed; + private string[] not_desired_usb_classes = { * not_desired -> undesired. I'd just call it 'usb_classes_blacklist' though. * This should be const (static is implied with const) and therefore declared at the top. @@ +21,3 @@ + // References for USB Class Codes can be seen here: + // http://www.usb.org/developers/defined_class + "0x03" // HID (Keyboards, Mice and Joysticks) So really no way to filter only keyboard and mouse? :( I think redirecting joystick would be a valid usecase. @@ +84,3 @@ } + private string build_usb_devices_filter () { * Please append private methods at the bottom please. Yeah, I know previous method isn't setting a good example. :) @@ +86,3 @@ + private string build_usb_devices_filter () { + // The filter used to determine which USB devices to expose consists of one or more rules, where each + // rule has the form of @class,@vendor,@product,@version,@allow and the rules themselves can be Why '@'? It only makes it ambigous IMO since it could be that '@' is part of the syntax while its not. @@ +89,3 @@ + // concatenad like @rule1|@rule2|@rule3. + // + // Using -1 for @class/@vendor/@product/@version means to accept any value. This is not really a valid use of '/'. You want to write "@class, @vendor.. or @version). I'd actually give these a name (e.g selector) in the previous para and just use that name here. @@ +91,3 @@ + // Using -1 for @class/@vendor/@product/@version means to accept any value. + // + // Our filter consits in filtering out the classes that are not interesting for Boxes (Class,-1,-1,-1,0) * in -> of * filtering out is redundant when you just mentioned you are talking about a filter. * The part in brackets is not so clear so just ditch that. @@ +92,3 @@ + // + // Our filter consits in filtering out the classes that are not interesting for Boxes (Class,-1,-1,-1,0) + // and finally concatenating a rule to accept all the other classes (-1,-1,-1,-1,1). so rules are short-circuited? i-e if the first rule matches, the next rule is not executed? You want to mention that here.
(In reply to comment #6) > Review of attachment 290252 [details] [review]: > > * What about tablets? Are they included? I guess tablets are included here as well. I don't have one here, handy, for testing how it is affected. > > So really no way to filter only keyboard and mouse? :( I think redirecting > joystick would be a valid usecase. This is pity. For each USB Class we have sub classes and protocols. We are not able to filter on that granularity (but maybe we should). The definition if the hardware is a keyboard or a mouse is in its protocol. One thing that can be done is get the list of the devices. Check whether they are "HID" (0x03), check whether the subclass is "Boot Interface Subclass" (0x01) and finally check the protocol to see whether they are a keyboard (0x01), mouse (0x02) or none (0x00). > @@ +92,3 @@ > + // > + // Our filter consits in filtering out the classes that are not > interesting for Boxes (Class,-1,-1,-1,0) > + // and finally concatenating a rule to accept all the other classes > (-1,-1,-1,-1,1). > > so rules are short-circuited? i-e if the first rule matches, the next rule is > not executed? You want to mention that here. AFAIU all rules are applied respecting the order used.
(In reply to comment #7) > (In reply to comment #6) > > Review of attachment 290252 [details] [review] [details]: > > > > > * What about tablets? Are they included? > > I guess tablets are included here as well. I don't have one here, handy, for > testing how it is affected. I don't have one either. You don't need to have one to find out what class it is. :) > > So really no way to filter only keyboard and mouse? :( I think redirecting > > joystick would be a valid usecase. > > This is pity. For each USB Class we have sub classes and protocols. We are not > able to filter on that granularity (but maybe we should). The definition if the > hardware is a keyboard or a mouse is in its protocol. Yes, I think spice should allow that. > One thing that can be done is get the list of the devices. Check whether they > are "HID" (0x03), check whether the subclass is "Boot Interface Subclass" > (0x01) and finally check the protocol to see whether they are a keyboard > (0x01), mouse (0x02) or none (0x00). You mean on Boxes side? I already thought of that but looking at the API docs, the device interface only has one method and that is to get a human readable description of the device. > > @@ +92,3 @@ > > + // > > + // Our filter consits in filtering out the classes that are not > > interesting for Boxes (Class,-1,-1,-1,0) > > + // and finally concatenating a rule to accept all the other classes > > (-1,-1,-1,-1,1). > > > > so rules are short-circuited? i-e if the first rule matches, the next rule is > > not executed? You want to mention that here. > > AFAIU all rules are applied respecting the order used. What does it mean exactly? Is the '|' and OR or AND?
(In reply to comment #8) > (In reply to comment #7) > > (In reply to comment #6) > > > Review of attachment 290252 [details] [review] [details] [details]: > > > > > > > > * What about tablets? Are they included? > > > > I guess tablets are included here as well. I don't have one here, handy, for > > testing how it is affected. > > I don't have one either. You don't need to have one to find out what class it > is. :) According to the docs, yes. Tablets are part of the HID class. > > > One thing that can be done is get the list of the devices. Check whether they > > are "HID" (0x03), check whether the subclass is "Boot Interface Subclass" > > (0x01) and finally check the protocol to see whether they are a keyboard > > (0x01), mouse (0x02) or none (0x00). > > You mean on Boxes side? I already thought of that but looking at the API docs, > the device interface only has one method and that is to get a human readable > description of the device. We only can do it on Boxes side if we add more APIs on spice-gtk providing us the info we need. > > > > @@ +92,3 @@ > > > + // > > > + // Our filter consits in filtering out the classes that are not > > > interesting for Boxes (Class,-1,-1,-1,0) > > > + // and finally concatenating a rule to accept all the other classes > > > (-1,-1,-1,-1,1). > > > > > > so rules are short-circuited? i-e if the first rule matches, the next rule is > > > not executed? You want to mention that here. > > > > AFAIU all rules are applied respecting the order used. > > What does it mean exactly? Is the '|' and OR or AND? Forget about what I said before about the rules. They are an AND.
(In reply to comment #9) > (In reply to comment #8) > > (In reply to comment #7) > > > (In reply to comment #6) > > > > Review of attachment 290252 [details] [review] [details] [details] [details]: > > > > > > @@ +92,3 @@ > > > > + // > > > > + // Our filter consits in filtering out the classes that are not > > > > interesting for Boxes (Class,-1,-1,-1,0) > > > > + // and finally concatenating a rule to accept all the other classes > > > > (-1,-1,-1,-1,1). > > > > > > > > so rules are short-circuited? i-e if the first rule matches, the next rule is > > > > not executed? You want to mention that here. > > > > > > AFAIU all rules are applied respecting the order used. > > > > What does it mean exactly? Is the '|' and OR or AND? > > Forget about what I said before about the rules. All of it? :) > They are an AND. If so, the rules you are setting up doesn't seem right. For one, why would you need to add the general rule for all devices?
(In reply to comment #10) > (In reply to comment #9) > > (In reply to comment #8) > > > (In reply to comment #7) > > > > (In reply to comment #6) > > > > > Review of attachment 290252 [details] [review] [details] [details] [details] [details]: > > > > > > > > @@ +92,3 @@ > > > > > + // > > > > > + // Our filter consits in filtering out the classes that are not > > > > > interesting for Boxes (Class,-1,-1,-1,0) > > > > > + // and finally concatenating a rule to accept all the other classes > > > > > (-1,-1,-1,-1,1). > > > > > > > > > > so rules are short-circuited? i-e if the first rule matches, the next rule is > > > > > not executed? You want to mention that here. > > > > > > > > AFAIU all rules are applied respecting the order used. > > > > > > What does it mean exactly? Is the '|' and OR or AND? > > > > Forget about what I said before about the rules. > > All of it? :) > > > They are an AND. > > If so, the rules you are setting up doesn't seem right. For one, why would you > need to add the general rule for all devices? My understanding of the rule can be wrong, but let's go: You want: - All devices from HID Class, doesn't matter vendor, product or version to be off (0x03,-1,-1,-1,0) AND - All other devices to be on (-1,-1,-1,-1,1) If you have another understanding of the filter (http://cgit.freedesktop.org/~jwrdegoede/usbredir/tree/usbredirparser/usbredirfilter.h), please, let me know.
(In reply to comment #11) > (In reply to comment #10) > > (In reply to comment #9) > > > (In reply to comment #8) > > > > (In reply to comment #7) > > > > > (In reply to comment #6) > > > > > > Review of attachment 290252 [details] [review] [details] [details] [details] [details] [details]: > > > > > > > > > > @@ +92,3 @@ > > > > > > + // > > > > > > + // Our filter consits in filtering out the classes that are not > > > > > > interesting for Boxes (Class,-1,-1,-1,0) > > > > > > + // and finally concatenating a rule to accept all the other classes > > > > > > (-1,-1,-1,-1,1). > > > > > > > > > > > > so rules are short-circuited? i-e if the first rule matches, the next rule is > > > > > > not executed? You want to mention that here. > > > > > > > > > > AFAIU all rules are applied respecting the order used. > > > > > > > > What does it mean exactly? Is the '|' and OR or AND? > > > > > > Forget about what I said before about the rules. > > > > All of it? :) > > > > > They are an AND. > > > > If so, the rules you are setting up doesn't seem right. For one, why would you > > need to add the general rule for all devices? > > My understanding of the rule can be wrong, but let's go: > You want: > - All devices from HID Class, doesn't matter vendor, product or version to be > off (0x03,-1,-1,-1,0) > AND > - All other devices to be on (-1,-1,-1,-1,1) I understand thats what you want to say but in the second rule you are saying "all devices" rather than "all other devices".
(In reply to comment #12) > (In reply to comment #11) > > (In reply to comment #10) > > > (In reply to comment #9) > > > > (In reply to comment #8) > > > > > (In reply to comment #7) > > > > > > (In reply to comment #6) > > > > > > > Review of attachment 290252 [details] [review] [details] [details] [details] [details] [details] [details]: > > > > > > > > > > > > @@ +92,3 @@ > > > > > > > + // > > > > > > > + // Our filter consits in filtering out the classes that are not > > > > > > > interesting for Boxes (Class,-1,-1,-1,0) > > > > > > > + // and finally concatenating a rule to accept all the other classes > > > > > > > (-1,-1,-1,-1,1). > > > > > > > > > > > > > > so rules are short-circuited? i-e if the first rule matches, the next rule is > > > > > > > not executed? You want to mention that here. > > > > > > > > > > > > AFAIU all rules are applied respecting the order used. > > > > > > > > > > What does it mean exactly? Is the '|' and OR or AND? > > > > > > > > Forget about what I said before about the rules. > > > > > > All of it? :) > > > > > > > They are an AND. > > > > > > If so, the rules you are setting up doesn't seem right. For one, why would you > > > need to add the general rule for all devices? > > > > My understanding of the rule can be wrong, but let's go: > > You want: > > - All devices from HID Class, doesn't matter vendor, product or version to be > > off (0x03,-1,-1,-1,0) > > AND > > - All other devices to be on (-1,-1,-1,-1,1) > > I understand thats what you want to say but in the second rule you are saying > "all devices" rather than "all other devices". I have stopped being lazy and took a look into the code (the docs are not explicit about this). The rules are short-circuited, as you said. So, if you have a HID device, it will match the "0x03,-1,-1,-1,0" rule and won't be allowed to redirect. If the device is not an HID, it will match the "-1,-1,-1,-1,1" rule and will be allowed. I'll update this info and resubmit the patch.
Created attachment 290391 [details] [review] Don't expose kbd & mice for USB redirection Create a function that allows us to create a filter (in the format documented in usbredir[0]) and use that to filter out the HID Class (keyboards, mice and joysticks). The decision to filter out the whole class was taken because there is no way to filter out only keyboards and mice. [0]: http://cgit.freedesktop.org/spice/usbredir/tree/usbredirparser/usbredirfilter.h
Review of attachment 290391 [details] [review]: Actually, I'd rather we don't merge this yet since its really not desirable to blacklist joysticks and tablets (keeping in mind that now user only explicitly enables redirection of individual devices) until we can modify it to only restrict mouse and keyboard. Now the review: * Shortlog is missing module prefix. "for USB redirection" can be omitted from shortlog if its making it long. * Doesn't make sense to use singular for kbd and plural for mouse. Just say mouse. * This is still valid from last review: "However it sounds like the function is generic and the caller is deciding to add the 'no HID' part to it, which isn't true." Since the function isn't generic and its private, there isn't really any need to mention it anyway. * Isn't the comment getting generated into some online API docs that you can link to from the log and also from the code? It'd be much better than a cgit link. ::: src/spice-display.vala @@ +365,3 @@ + // not executed). + // + // Using -1 for class,vendor,product,version means to accept any value. ",version" -> ", version" and at least in English, you put a space after every comma. @@ +368,3 @@ + // + // Our filter consists of removing all classes that are not interesting for Boxes and finally concatenating + // a rule that allows all possible classes. Good to have this documentation here. I'm assuming there is no link to any spice docs that explain this? If there is, please just link to that instead.
(In reply to comment #15) > Review of attachment 290391 [details] [review]: > > Actually, I'd rather we don't merge this yet since its really not desirable to > blacklist joysticks and tablets (keeping in mind that now user only explicitly > enables redirection of individual devices) until we can modify it to only > restrict mouse and keyboard. :-( I have tested it with a cellphone and an iPad plugged in and both can be redirect to the Guest with the patch. Talking about joysticks, on one hand I understand your point; on the other hand I don't see people redirecting joysticks to play games that soon. At least not till we have GPU pass through/3D support. > > @@ +368,3 @@ > + // > + // Our filter consists of removing all classes that are not > interesting for Boxes and finally concatenating > + // a rule that allows all possible classes. > > Good to have this documentation here. I'm assuming there is no link to any > spice docs that explain this? If there is, please just link to that instead. We don't have this doc on Spice (maybe we don't have this anywhere but here).
(In reply to comment #15) > Review of attachment 290391 [details] [review]: > > * Isn't the comment getting generated into some online API docs that you can > link to from the log and also from the code? It'd be much better than a cgit > link. I wasn't able to find one.
Created attachment 290403 [details] [review] spice-display: Don't expose kbd & mouse Filter out the HID Class (keyboards, mice and joysticks). The decision to filter out the whole class was taken because there is no way to filter out only keyboards and mice. [0]: http://cgit.freedesktop.org/spice/usbredir/tree/usbredirparser/usbredirfilter.h
(In reply to comment #16) > (In reply to comment #15) > > Review of attachment 290391 [details] [review] [details]: > > > > Actually, I'd rather we don't merge this yet since its really not desirable to > > blacklist joysticks and tablets (keeping in mind that now user only explicitly > > enables redirection of individual devices) until we can modify it to only > > restrict mouse and keyboard. > > :-( Don't be sad. I'm not rejecting the patch, just delaying it till it can be changed slightly to correctly solve the issue. > I have tested it with a cellphone and an iPad plugged in and both can be > redirect to the Guest with the patch. Thanks for testing but that only shows the patch doesn't break non-HID devices. > Talking about joysticks, on one hand I understand your point; on the other hand > I don't see people redirecting joysticks to play games that soon. At least not > till we have GPU pass through/3D support. * You are underestimating people's will to play old (arcade) games in VMs. :) Think about all those awesome DOS games.. * If there really is no way to specify mouse and keyboard only, I could consider taking this patch as is but as you pointed out yourself, spice could be hacked to support better filters or give us info that enables us to do that in Boxes. * What about tablets? Its also possible we are missing some other HID devices for which it doesn't make sense to filter out here. > > @@ +368,3 @@ > > + // > > + // Our filter consists of removing all classes that are not > > interesting for Boxes and finally concatenating > > + // a rule that allows all possible classes. > > > > Good to have this documentation here. I'm assuming there is no link to any > > spice docs that explain this? If there is, please just link to that instead. > > We don't have this doc on Spice (maybe we don't have this anywhere but here). Nice work Hans! :(
Created attachment 290419 [details] [review] spice-display: Don't expose kbd & mouse Filter out the HID Class (keyboards, mice and joysticks). The decision to filter out the whole class was taken because there is no way to filter out only keyboards and mice. [0]: http://cgit.freedesktop.org/spice/usbredir/tree/usbredirparser/usbredirfilter.h
(In reply to comment #20) > Created an attachment (id=290419) [details] [review] > spice-display: Don't expose kbd & mouse > > Filter out the HID Class (keyboards, mice and joysticks). The decision to > filter out the whole class was taken because there is no way to filter > out only keyboards and mice. > > [0]: > http://cgit.freedesktop.org/spice/usbredir/tree/usbredirparser/usbredirfilter.h Ah, this patch depends on http://lists.freedesktop.org/archives/spice-devel/2014-November/017887.html (Yeah, then I'll have to bump the spice-gtk version that Boxes depends on, I know).
(In reply to comment #21) > (In reply to comment #20) > > Created an attachment (id=290419) [details] [review] [details] [review] > > spice-display: Don't expose kbd & mouse > > > > Filter out the HID Class (keyboards, mice and joysticks). The decision to > > filter out the whole class was taken because there is no way to filter > > out only keyboards and mice. > > > > [0]: > > http://cgit.freedesktop.org/spice/usbredir/tree/usbredirparser/usbredirfilter.h > > Ah, this patch depends on > http://lists.freedesktop.org/archives/spice-devel/2014-November/017887.html > > (Yeah, then I'll have to bump the spice-gtk version that Boxes depends on, I > know). The less intrusive/more generic solution we can have on spice-gtk side is: http://lists.freedesktop.org/archives/spice-devel/2014-November/017928.html I'll update a patch that follows the accepted solution
Created attachment 291005 [details] [review] spice-display: Don't expose kbd & mouse The way chosen for do not expose keyboard and mouse is: - Get a list of all present devices but the ones part of HID Class (keyboard, mouse, tablet, josytick, ...). - Get a list of all present devices part of HID Class. - For each HID Class device, check if it is: - a keyboard or mouse; - one of its interfaces represents a keyboard or mouse - If neither the device (nor one of its interfaces) is a keyboard or a mouse, the devices is added to the list of devices that are going to be exposed.
Review of attachment 291005 [details] [review]: I think commit description could be very simple if the filtering code is simple. In fact you don't need to explain the code at all but rather provide rationale (which should be provided anyway). ::: configure.ac @@ +59,3 @@ LIBXML2_MIN_VERSION=2.7.8 +LIBUSB_MIN_VERSION=1.0.9 +SPICE_GTK_MIN_VERSION=0.26.18 Please put them in a separate patch. I like to keep dep bumps into separate patches anyway but if we are adding a new dep, its even more important then. ::: src/spice-display.vala @@ +371,3 @@ + // Class but neither a Keyboard nor a Mouse. + + var devs = manager.get_devices_with_filter ("0x03,-1,-1,-1,0|-1,-1,-1,-1,1"); instead of getting the list twice with different filters and then doing our own filtering etc, the code will be a lot simpler if you just get all devices and then filter them. @@ +383,3 @@ + continue; + + if (desc.bDeviceClass == LibUSB.ClassCode.HID) { I think a simple if statement checking for both HID and protocols in the same conditions would suffice and would result in less code.
(In reply to comment #24) > Review of attachment 291005 [details] [review]: > > I think commit description could be very simple if the filtering code is > simple. In fact you don't need to explain the code at all but rather provide > rationale (which should be provided anyway). > > ::: configure.ac > @@ +59,3 @@ > LIBXML2_MIN_VERSION=2.7.8 > +LIBUSB_MIN_VERSION=1.0.9 > +SPICE_GTK_MIN_VERSION=0.26.18 > > Please put them in a separate patch. I like to keep dep bumps into separate > patches anyway but if we are adding a new dep, its even more important then. Okay. > > ::: src/spice-display.vala > @@ +371,3 @@ > + // Class but neither a Keyboard nor a Mouse. > + > + var devs = manager.get_devices_with_filter > ("0x03,-1,-1,-1,0|-1,-1,-1,-1,1"); > > instead of getting the list twice with different filters and then doing our own > filtering etc, the code will be a lot simpler if you just get all devices and > then filter them. As you prefer. But I truly believe this way is better for understanding. > > @@ +383,3 @@ > + continue; > + > + if (desc.bDeviceClass == LibUSB.ClassCode.HID) { > > I think a simple if statement checking for both HID and protocols in the same > conditions would suffice and would result in less code. It would result in less code, for sure. And the small amount of code would be way less clear than it is now. I do not agree about changing this.
(In reply to comment #25) > (In reply to comment #24) > > Review of attachment 291005 [details] [review] [details]: > > ::: src/spice-display.vala > > @@ +371,3 @@ > > + // Class but neither a Keyboard nor a Mouse. > > + > > + var devs = manager.get_devices_with_filter > > ("0x03,-1,-1,-1,0|-1,-1,-1,-1,1"); > > > > instead of getting the list twice with different filters and then doing our own > > filtering etc, the code will be a lot simpler if you just get all devices and > > then filter them. > > As you prefer. > But I truly believe this way is better for understanding. Arguments? How is it more clear? > > > > @@ +383,3 @@ > > + continue; > > + > > + if (desc.bDeviceClass == LibUSB.ClassCode.HID) { > > > > I think a simple if statement checking for both HID and protocols in the same > > conditions would suffice and would result in less code. > > It would result in less code, for sure. And the small amount of code would be > way less clear than it is now. I do not agree about changing this. Same here. Maybe if you could explain, I might get convinced but just stating you disagree neither convinces you nor me.
Created attachment 291025 [details] [review] spice-display: Don't expose kbd & mouse As the device being redirect to the guest is not available in the host, do not expose keyboards and mice to the user.
(In reply to comment #26) > (In reply to comment #25) > > (In reply to comment #24) > > > Review of attachment 291005 [details] [review] [details] [details]: > > > ::: src/spice-display.vala > > > @@ +371,3 @@ > > > + // Class but neither a Keyboard nor a Mouse. > > > + > > > + var devs = manager.get_devices_with_filter > > > ("0x03,-1,-1,-1,0|-1,-1,-1,-1,1"); > > > > > > instead of getting the list twice with different filters and then doing our own > > > filtering etc, the code will be a lot simpler if you just get all devices and > > > then filter them. > > > > As you prefer. > > But I truly believe this way is better for understanding. > > Arguments? How is it more clear? I agree about this part being more clear using your way ... > > > > > > > @@ +383,3 @@ > > > + continue; > > > + > > > + if (desc.bDeviceClass == LibUSB.ClassCode.HID) { > > > > > > I think a simple if statement checking for both HID and protocols in the same > > > conditions would suffice and would result in less code. > > > > It would result in less code, for sure. And the small amount of code would be > > way less clear than it is now. I do not agree about changing this. > > Same here. Maybe if you could explain, I might get convinced but just stating > you disagree neither convinces you nor me. But here I don't, seriously. As you can see in the attached patch, I had to add a *lot* of comments to have it more or less clear for a person that never played with USB before. Also, if something change (being added) in this specific part of USB spec, you probably will have to rework a bit in this code. In the previously code I'm pretty sure it would be just one more line added in each switch. As you're the maintainer, you can choose between the way you prefer the patch, but it is way less readable than before.
(In reply to comment #27) > Created an attachment (id=291025) [details] [review] > spice-display: Don't expose kbd & mouse > > As the device being redirect to the guest is not available in the host, > do not expose keyboards and mice to the user. Ouch, and I forgot to split the patch. I'll attach another one.
Created attachment 291042 [details] [review] Add libusb dependency Boxes is going to use libusb_device for filtering out keyboard and mouse
Created attachment 291043 [details] [review] Bump spice-gtk dependency Boxes is going to use an API to have access to libusb device that will be introduced in 0.27 release.
Created attachment 291044 [details] [review] spice-display: Don't expose kbd & mouse As the device being redirect to the guest is not available in the host, do not expose keyboards and mice to the user.
Created attachment 291098 [details] [review] spice-display: Don't expose kbd & mouse -- alternative version Alternative version, using switches instead of if-else, for filtering out USB keyboard and mouse
Review of attachment 291042 [details] [review]: Please add a '.' at the end of description before pushing.
Review of attachment 291043 [details] [review]: 'to have access to' -> 'to access underlying'.
Created attachment 291101 [details] [review] spice-display: Don't expose kbd & mouse - alternative version
Review of attachment 291101 [details] [review]: In general, sure lets go with this approach but it still will need some work. ::: src/spice-display.vala @@ +360,3 @@ // - Check if the devices in HID Class are Keyboards [HID Class (0x03), Boot Interface Subclass (0x01), // Interface Protocol: Keyboard (0x01)] or Mice [HID Class (0x03), Boot Interface Subclass (0x01), Interface + // Protocol: Mouse (0x02)]. If they are neither keyboard nor mouse, add them to the list of devices to be Please fix this typo in the previous patch as its nothing to do with alternative approach. @@ +369,3 @@ var libusb_dev = (LibUSB.Device) dev.get_libusb_device (); + // The info we need can be in the device descriptor ... irrelevant to alternative approach as well. @@ -373,3 @@ continue; - // Class, SubClass and Protocol can be found either in the interface descriptor or in the device descriptor How does this comment become obsolete with move from if/else to switch? @@ -391,3 @@ - // 0x00: None, 0x01: Keyboard, 0x02: Mouse - // So, the device only has to be added if its SubClass value is 0x01 and its Protocol value is - // 0x00, otherwise we just ignore the device. If this big comment is needed, its needed for both cases. For the if/else approach, I already told you that code will look a lot cleaner if: * if put "config.@interface[j].altsetting[k].*" into temp variables and * Add the same simple comments next to the conditions on the same line like you do in this alternative version (e.g // Keyboard).
(In reply to comment #37) > Review of attachment 291101 [details] [review]: > > In general, sure lets go with this approach but it still will need some work. > > ::: src/spice-display.vala > @@ +360,3 @@ > // - Check if the devices in HID Class are Keyboards [HID Class > (0x03), Boot Interface Subclass (0x01), > // Interface Protocol: Keyboard (0x01)] or Mice [HID Class (0x03), > Boot Interface Subclass (0x01), Interface > + // Protocol: Mouse (0x02)]. If they are neither keyboard nor mouse, > add them to the list of devices to be > > Please fix this typo in the previous patch as its nothing to do with > alternative approach. > > @@ +369,3 @@ > var libusb_dev = (LibUSB.Device) dev.get_libusb_device (); > > + // The info we need can be in the device descriptor ... > > irrelevant to alternative approach as well. Is it? Not sure because ... > > @@ -373,3 @@ > continue; > > - // Class, SubClass and Protocol can be found either in the > interface descriptor or in the device descriptor > > How does this comment become obsolete with move from if/else to switch? The comment above (plus this one: // Or in one of the interfaces descriptors) is the reason why the comment became obsolete moving from the previous approach to this one. > > @@ -391,3 @@ > - // 0x00: None, 0x01: Keyboard, 0x02: Mouse > - // So, the device only has to be added if its SubClass > value is 0x01 and its Protocol value is > - // 0x00, otherwise we just ignore the device. > > If this big comment is needed, its needed for both cases. It's not needed in both cases. In the alternative patch the switch is more than clear about *all* possible cases and what each value means: switch (desc.bDeviceSubClass) { case 0x00: // None ... case 0x01: // BootInterface switch (desc.bDeviceProtocol) { case 0x00: // None ... case 0x01: // Keyboard case 0x02: // Mouse ... } } On the other hand, when we are working if the if-else approach, unless you put if-elses exactly like the switch constructor you have to have additional info to understand why comparing to 0x00 or 0x01 ... provided by that big comment there. > For the if/else > approach, I already told you that code will look a lot cleaner if: > > * if put "config.@interface[j].altsetting[k].*" into temp variables and > * Add the same simple comments next to the conditions on the same line like you > do in this alternative version (e.g // Keyboard). It can be valid for both actually
(In reply to comment #38) > (In reply to comment #37) > > Review of attachment 291101 [details] [review] [details]: > > > > @@ +369,3 @@ > > var libusb_dev = (LibUSB.Device) dev.get_libusb_device (); > > > > + // The info we need can be in the device descriptor ... > > > > irrelevant to alternative approach as well. > > Is it? Not sure because ... > > > > > @@ -373,3 @@ > > continue; > > > > - // Class, SubClass and Protocol can be found either in the > > interface descriptor or in the device descriptor > > > > How does this comment become obsolete with move from if/else to switch? > > The comment above (plus this one: // Or in one of the interfaces descriptors) > is the reason why the comment became obsolete moving from the previous approach > to this one. Ah! The comments are so far from each other that I don't think reader will easily see that they are related. This makes me thing about something I should have before: This function is getting really big and complicated so why not put at least the code to get the needed info into separate functions? That will not only improve the readability a lot, it will also, as a nice side-effect, put these two related comments near to each other so that reader will not miss the relations. > > > > @@ -391,3 @@ > > - // 0x00: None, 0x01: Keyboard, 0x02: Mouse > > - // So, the device only has to be added if its SubClass > > value is 0x01 and its Protocol value is > > - // 0x00, otherwise we just ignore the device. > > > > If this big comment is needed, its needed for both cases. > > It's not needed in both cases. In the alternative patch the switch is more than > clear about *all* possible cases and what each value means: As I pointed out before, while you are *likely* correct that switch in this case is clearer, it doesn't make need for explaining of protocol redundant. I already explained more than once, how you can achieve exactly the same with if/else too: var some_info = ..; var another_info = ..; If (some_info == 0x00 && // Device class XYZ another_info == 0x01) // protocol ABC Anyway, this is academic now that we are going to go with switch anyway.
Created attachment 291120 [details] [review] spice-display: Don't expose kbd & mouse As the device being redirect to the guest is not available in the host, do not expose keyboards and mice to the user.
Review of attachment 291120 [details] [review]: Almost there! To make it even more clearer: "As the device being redirect to the guest is not available in the host" -> "As a USB device is no longer available on the host after being redirected to guest". ::: src/spice-display.vala @@ +289,3 @@ try { var manager = UsbDeviceManager.get (session); + var devs = get_usb_devices(manager); nitpick: Space before '('. @@ +355,3 @@ } + + private bool is_kbd_or_mouse (uint8 class, uint8 subclass, uint8 protocol) { nitpicks: * Can we name it 'is_usb_kbd_or_mouse'? * Can we move this below? The convention i've been trying to follow is to have the caller before callee. @@ +371,3 @@ + case 0x02: // Mouse + ret = true; + break; what about other cases?
Review of attachment 291120 [details] [review]: ::: src/spice-display.vala @@ +355,3 @@ } + + private bool is_kbd_or_mouse (uint8 class, uint8 subclass, uint8 protocol) { Sure, for both questions. @@ +371,3 @@ + case 0x02: // Mouse + ret = true; + break; There are no other cases. http://www.usblyzer.com/usb-human-interface-device-hid-class-decoder.htm
Created attachment 291123 [details] [review] spice-display: Don't expose kbd & mouse As a USB device is no longer available on the host after being redirected to guest, do not expose keyboards and mice to the user.
(In reply to comment #42) > Review of attachment 291120 [details] [review]: > > @@ +371,3 @@ > + case 0x02: // Mouse > + ret = true; > + break; > > There are no other cases. > http://www.usblyzer.com/usb-human-interface-device-hid-class-decoder.htm Oh? Are we sure then that 01 and 02 only mean kbd and mouse very specifically and not other HID devices (ones we didn't want to filter)?
Review of attachment 291123 [details] [review]: Apart from these tiny things, its good. I'll wait for your answer to my previous query though before marking it as commitnow. ::: src/spice-display.vala @@ +359,3 @@ + var devs = manager.get_devices (); + + for (int i = 0; i < devs.length; i++) { oh, one more really tiny request: Can we please please use foreach here instead? They are slightly more readable than for. @@ +378,3 @@ + + var kbd_or_mouse = false; + for (int j = 0; j < config.@interface.length && !kbd_or_mouse; j++) { if you go for foreach, you'd want to replace j with i and k with j.
Review of attachment 291123 [details] [review]: ::: src/spice-display.vala @@ +359,3 @@ + var devs = manager.get_devices (); + + for (int i = 0; i < devs.length; i++) { GLib.GenericArray<Spice.UsbDevice>' does not have an `iterator' @@ +378,3 @@ + + var kbd_or_mouse = false; + if (libusb_dev.get_device_descriptor (out desc) != 0) It won't be the case :-)
(In reply to comment #44) > (In reply to comment #42) > > Review of attachment 291120 [details] [review] [details]: > > > > @@ +371,3 @@ > > + case 0x02: // Mouse > > + ret = true; > > + break; > > > > There are no other cases. > > http://www.usblyzer.com/usb-human-interface-device-hid-class-decoder.htm > > Oh? Are we sure then that 01 and 02 only mean kbd and mouse very specifically > and not other HID devices (ones we didn't want to filter)? This is exactly what I understand from the link I've posted above.
(In reply to comment #47) > (In reply to comment #44) > > (In reply to comment #42) > > > Review of attachment 291120 [details] [review] [details] [details]: > > > > > > @@ +371,3 @@ > > > + case 0x02: // Mouse > > > + ret = true; > > > + break; > > > > > > There are no other cases. > > > http://www.usblyzer.com/usb-human-interface-device-hid-class-decoder.htm > > > > Oh? Are we sure then that 01 and 02 only mean kbd and mouse very specifically > > and not other HID devices (ones we didn't want to filter)? > > This is exactly what I understand from the link I've posted above. Well it does not say 0x is for all devices but only that it means 'none'. I don't think that table there is a comprehensive list of values. /usr/share/hwdata/usb.ids seems to list many protocols, even just under HID class.
(In reply to comment #48) > (In reply to comment #47) > > (In reply to comment #44) > > > (In reply to comment #42) > > > > Review of attachment 291120 [details] [review] [details] [details] [details]: > > > > > > > > @@ +371,3 @@ > > > > + case 0x02: // Mouse > > > > + ret = true; > > > > + break; > > > > > > > > There are no other cases. > > > > http://www.usblyzer.com/usb-human-interface-device-hid-class-decoder.htm > > > > > > Oh? Are we sure then that 01 and 02 only mean kbd and mouse very specifically > > > and not other HID devices (ones we didn't want to filter)? > > > > This is exactly what I understand from the link I've posted above. > > Well it does not say 0x is for all devices but only that it means 'none'. I > don't think that table there is a comprehensive list of values. > /usr/share/hwdata/usb.ids seems to list many protocols, even just under HID > class. I think I'm probably misreading that so nm that. The reference document about HID also only lists 3 values but it does reserve many values so to be future safe, I'd feel better if we replace the 0x branch with a default branch.
(In reply to comment #49) > (In reply to comment #48) > > (In reply to comment #47) > > > (In reply to comment #44) > > > > (In reply to comment #42) > > > > > Review of attachment 291120 [details] [review] [details] [details] [details] [details]: > > > > > > > > > > @@ +371,3 @@ > > > > > + case 0x02: // Mouse > > > > > + ret = true; > > > > > + break; > > > > > > > > > > There are no other cases. > > > > > http://www.usblyzer.com/usb-human-interface-device-hid-class-decoder.htm > > > > > > > > Oh? Are we sure then that 01 and 02 only mean kbd and mouse very specifically > > > > and not other HID devices (ones we didn't want to filter)? > > > > > > This is exactly what I understand from the link I've posted above. > > > > Well it does not say 0x is for all devices but only that it means 'none'. I > > don't think that table there is a comprehensive list of values. > > /usr/share/hwdata/usb.ids seems to list many protocols, even just under HID > > class. > > I think I'm probably misreading that so nm that. The reference document about > HID also only lists 3 values but it does reserve many values so to be future > safe, I'd feel better if we replace the 0x branch with a default branch. What you're seeing is the vendor/product ID. I'm fine with replacing the 0x00 branch with the default one. But I'd prefer to keep the 0x00 as it is now, adding the default branch and having a g_warn_if_reach() there (I'd be satisfied with a debug() though), just to make our life easier in case of something new is added and we have to debug it.
(In reply to comment #50) > (In reply to comment #49) > > (In reply to comment #48) > > > (In reply to comment #47) > > > > (In reply to comment #44) > > > > > (In reply to comment #42) > > > > > > Review of attachment 291120 [details] [review] [details] [details] [details] [details] [details]: > > > > > > > > > > > > @@ +371,3 @@ > > > > > > + case 0x02: // Mouse > > > > > > + ret = true; > > > > > > + break; > > > > > > > > > > > > There are no other cases. > > > > > > http://www.usblyzer.com/usb-human-interface-device-hid-class-decoder.htm > > > > > > > > > > Oh? Are we sure then that 01 and 02 only mean kbd and mouse very specifically > > > > > and not other HID devices (ones we didn't want to filter)? > > > > > > > > This is exactly what I understand from the link I've posted above. > > > > > > Well it does not say 0x is for all devices but only that it means 'none'. I > > > don't think that table there is a comprehensive list of values. > > > /usr/share/hwdata/usb.ids seems to list many protocols, even just under HID > > > class. > > > > I think I'm probably misreading that so nm that. The reference document about > > HID also only lists 3 values but it does reserve many values so to be future > > safe, I'd feel better if we replace the 0x branch with a default branch. > > What you're seeing is the vendor/product ID. > I'm fine with replacing the 0x00 branch with the default one. But I'd prefer to > keep the 0x00 as it is now, adding the default branch and having a > g_warn_if_reach() there (I'd be satisfied with a debug() though), just to make > our life easier in case of something new is added and we have to debug it. As I said on IRC the default case does not imply an issues. I don't think Boxes will be used to debug untested new USB devices. Anyway, feel free to add a debug there.
Created attachment 292539 [details] [review] Add libusb dependency Boxes is going to use libusb_device for filtering out keyboard and mouse.
Created attachment 292540 [details] [review] Bump spice-gtk dependency Boxes is going to use an API to access underlying libusb device that was introduced in 0.27 release.
Created attachment 292549 [details] [review] spice-display: Don't expose kbd & mouse As a USB device is no longer available on the host after being redirected to guest, do not expose keyboards and mice to the user.
Review of attachment 292539 [details] [review]: ACK
Review of attachment 292540 [details] [review]: ack
Review of attachment 292549 [details] [review]: ACK if you tested this particular version.
Created attachment 292643 [details] [review] spice-display: Don't expose kbd & mouse As a USB device is no longer available on the host after being redirected to guest, do not expose keyboards and mice to the user.
Attachment 292539 [details] pushed as 62aae3b - Add libusb dependency Attachment 292540 [details] pushed as cd8252f - Bump spice-gtk dependency Attachment 292643 [details] pushed as 8f8f588 - spice-display: Don't expose kbd & mouse