[Spice-devel] [spice-gtk v2 1/8] usb-redir: define interfaces to support emulated devices
Yuri Benditovich
yuri.benditovich at daynix.com
Thu Aug 8 10:35:56 UTC 2019
On Thu, Aug 8, 2019 at 12:46 PM Frediano Ziglio <fziglio at redhat.com> wrote:
>
> > On Thu, Aug 8, 2019 at 9:54 AM Frediano Ziglio <fziglio at redhat.com> wrote:
> > >
> > > > On Wed, Aug 7, 2019 at 6:21 PM Frediano Ziglio <fziglio at redhat.com>
> > > > wrote:
> > > > >
> > > > > >
> > > > > > On Mon, Aug 5, 2019 at 3:17 PM Frediano Ziglio <fziglio at redhat.com>
> > > > > > wrote:
> > > > > > >
> > > > > > > >
> > > > > > > > On Mon, Aug 5, 2019 at 12:58 PM Frediano Ziglio
> > > > > > > > <fziglio at redhat.com>
> > > > > > > > wrote:
> > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > SpiceUsbBackendDevice structure is extended to support
> > > > > > > > > > additional kind of device that is emulated by Spice-GTK
> > > > > > > > > > and not present locally (and does not have libusb_device),
> > > > > > > > > > such device has instead pointer to SpiceUsbEmulatedDevice
> > > > > > > > > > abstract structure. Specific implementation of such device
> > > > > > > > > > depends on its device type. Implementation module will define
> > > > > > > > > > constructor for specific device type.
> > > > > > > > > > Device structure is abstract but always starts from table of
> > > > > > > > > > virtual functions required to redirect such virtual device.
> > > > > > > > > >
> > > > > > > > > > Signed-off-by: Yuri Benditovich <yuri.benditovich at daynix.com>
> > > > > > > > > > ---
> > > > > > > > > > src/meson.build | 1 +
> > > > > > > > > > src/usb-backend.c | 102
> > > > > > > > > > +++++++++++++++++++++++++++++++++++++++++++-
> > > > > > > > > > src/usb-backend.h | 3 ++
> > > > > > > > > > src/usb-emulation.h | 91
> > > > > > > > > > +++++++++++++++++++++++++++++++++++++++
> > > > > > > > > > 4 files changed, 195 insertions(+), 2 deletions(-)
> > > > > > > > > > create mode 100644 src/usb-emulation.h
> > > > > > > > > >
> > > > > > > > > > diff --git a/src/meson.build b/src/meson.build
> > > > > > > > > > index b4a6870..fe19c16 100644
> > > > > > > > > > --- a/src/meson.build
> > > > > > > > > > +++ b/src/meson.build
> > > > > > > > > > @@ -122,6 +122,7 @@ spice_client_glib_sources = [
> > > > > > > > > > 'usbutil.c',
> > > > > > > > > > 'usbutil.h',
> > > > > > > > > > 'usb-backend.c',
> > > > > > > > > > + 'usb-emulation.h',
> > > > > > > > > > 'usb-backend.h',
> > > > > > > > > > 'vmcstream.c',
> > > > > > > > > > 'vmcstream.h',
> > > > > > > > > > diff --git a/src/usb-backend.c b/src/usb-backend.c
> > > > > > > > > > index 3334f56..be60cf3 100644
> > > > > > > > > > --- a/src/usb-backend.c
> > > > > > > > > > +++ b/src/usb-backend.c
> > > > > > > > > > @@ -39,6 +39,7 @@
> > > > > > > > > > #include "usbredirparser.h"
> > > > > > > > > > #include "spice-util.h"
> > > > > > > > > > #include "usb-backend.h"
> > > > > > > > > > +#include "usb-emulation.h"
> > > > > > > > > > #include "channel-usbredir-priv.h"
> > > > > > > > > > #include "spice-channel-priv.h"
> > > > > > > > > >
> > > > > > > > > > @@ -46,7 +47,10 @@
> > > > > > > > > >
> > > > > > > > > > struct _SpiceUsbBackendDevice
> > > > > > > > > > {
> > > > > > > > > > + /* Pointer to device. Either real device (libusb_device)
> > > > > > > > > > + * or emulated one (edev) */
> > > > > > > > > > libusb_device *libusb_device;
> > > > > > > > > > + SpiceUsbEmulatedDevice *edev;
> > > > > > > > > > gint ref_count;
> > > > > > > > > > SpiceUsbBackendChannel *attached_to;
> > > > > > > > > > UsbDeviceInformation device_info;
> > > > > > > > > > @@ -66,6 +70,10 @@ struct _SpiceUsbBackend
> > > > > > > > > > libusb_device **libusb_device_list;
> > > > > > > > > > gint redirecting;
> > > > > > > > > > #endif
> > > > > > > > > > +
> > > > > > > > > > + /* Mask of allocated device, a specific bit set to 1 to
> > > > > > > > > > indicate
> > > > > > > > > > that
> > > > > > > > > > the device at
> > > > > > > > > > + * that address is allocated */
> > > > > > > > > > + uint32_t own_devices_mask;
> > > > > > > > > > };
> > > > > > > > > >
> > > > > > > > > > struct _SpiceUsbBackendChannel
> > > > > > > > > > @@ -413,6 +421,8 @@ SpiceUsbBackend
> > > > > > > > > > *spice_usb_backend_new(GError
> > > > > > > > > > **error)
> > > > > > > > > > libusb_set_option(be->libusb_context,
> > > > > > > > > > LIBUSB_OPTION_USE_USBDK);
> > > > > > > > > > #endif
> > > > > > > > > > #endif
> > > > > > > > > > + /* exclude addresses 0 (reserved) and 1 (root hub)
> > > > > > > > > > */
> > > > > > > > > > + be->own_devices_mask = 3;
> > > > > > > > > > }
> > > > > > > > > > SPICE_DEBUG("%s <<", __FUNCTION__);
> > > > > > > > > > return be;
> > > > > > > > > > @@ -529,8 +539,13 @@ void
> > > > > > > > > > spice_usb_backend_device_unref(SpiceUsbBackendDevice *dev)
> > > > > > > > > > {
> > > > > > > > > > LOUD_DEBUG("%s >> %p(%d)", __FUNCTION__, dev,
> > > > > > > > > > dev->ref_count);
> > > > > > > > > > if (g_atomic_int_dec_and_test(&dev->ref_count)) {
> > > > > > > > > > - libusb_unref_device(dev->libusb_device);
> > > > > > > > > > - LOUD_DEBUG("%s freeing %p (libusb %p)",
> > > > > > > > > > __FUNCTION__,
> > > > > > > > > > dev,
> > > > > > > > > > dev->libusb_device);
> > > > > > > > > > + if (dev->libusb_device) {
> > > > > > > > > > + libusb_unref_device(dev->libusb_device);
> > > > > > > > > > + LOUD_DEBUG("%s freeing %p (libusb %p)",
> > > > > > > > > > __FUNCTION__,
> > > > > > > > > > dev,
> > > > > > > > > > dev->libusb_device);
> > > > > > > > > > + }
> > > > > > > > > > + if (dev->edev) {
> > > > > > > > > > + device_ops(dev->edev)->unrealize(dev->edev);
> > > > > > > > > > + }
> > > > > > > > > > g_free(dev);
> > > > > > > > > > }
> > > > > > > > > > }
> > > > > > > > > > @@ -829,4 +844,87 @@
> > > > > > > > > > spice_usb_backend_channel_get_guest_filter(SpiceUsbBackendChannel
> > > > > > > > > > *ch,
> > > > > > > > > > }
> > > > > > > > > > }
> > > > > > > > > >
> > > > > > > > > > +void spice_usb_backend_device_report_change(SpiceUsbBackend
> > > > > > > > > > *be,
> > > > > > > > > > +
> > > > > > > > > > SpiceUsbBackendDevice
> > > > > > > > > > *dev)
> > > > > > > > > > +{
> > > > > > > > > > + gchar *desc;
> > > > > > > > > > + g_return_if_fail(dev && dev->edev);
> > > > > > > > > > +
> > > > > > > > > > + desc =
> > > > > > > > > > device_ops(dev->edev)->get_product_description(dev->edev);
> > > > > > > > > > + SPICE_DEBUG("%s: %s", __FUNCTION__, desc);
> > > > > > > > > > + g_free(desc);
> > > > > > > > > > +}
> > > > > > > > > > +
> > > > > > > > >
> > > > > > > > > Looks like this function is just for debugging.
> > > > > > > > > Why instead use spice_usb_backend_device_get_description in
> > > > > > > > > usb-device-cd.c
> > > > > > > > > and call SPICE_DEBUG directly?
> > > > > > > >
> > > > > > > > usb-device-cd.c has nothing to do with this change.
> > > > > > > > It issues the update to whom it might be important.
> > > > > > > > Currently as we do not have UI, also outside of usb-device-cd.c
> > > > > > > > there
> > > > > > > > is no action.
> > > > > > > >
> > > > > > >
> > > > > > > It sounds fine. I found the name not much meaningful. Looking at
> > > > > > > the
> > > > > > > code
> > > > > > > this is a state change (stop or start), the name indicate a generic
> > > > > > > "change". Maybe "spice_usb_backend_device_state_changed" would be
> > > > > > > more
> > > > > > > meaningful?
> > > > > > >
> > > > > > > > >
> > > > > > > > > > +void spice_usb_backend_device_eject(SpiceUsbBackend *be,
> > > > > > > > > > SpiceUsbBackendDevice *dev)
> > > > > > > > > > +{
> > > > > > > > > > + g_return_if_fail(dev && dev->edev);
> > > > > > > > > > +
> > > > > > > > > > + if (be->hotplug_callback) {
> > > > > > > > > > + be->hotplug_callback(be->hotplug_user_data, dev,
> > > > > > > > > > FALSE);
> > > > > > > > > > + }
> > > > > > > > > > + be->own_devices_mask &= ~(1 <<
> > > > > > > > > > dev->device_info.address);
> > > > > > > > > > +
> > > > > > > > > > + spice_usb_backend_device_unref(dev);
> > > > > > > > >
> > > > > > > > > From my experiments looks like that reference counting for
> > > > > > > > > these
> > > > > > > > > emulated devices are different from normal ones.
> > > > > > > > > In normal ones the device list in usb manager is the main
> > > > > > > > > owner,
> > > > > > > > > for these devices you have an additional reference that is
> > > > > > > > > removed
> > > > > > > > > here. So if this function is not called you have a leak. Also
> > > > > > > > > is weird to have a reference without having an actual pointer.
> > > > > > > > > The same apply to own_devices_mask bit clearance (added in this
> > > > > > > > > version of the patch). If I set the bit creating an object I
> > > > > > > > > expect
> > > > > > > > > the bit to be clear during the object destruction, not another
> > > > > > > > > not related function. I wrote a test that call in a loop 128
> > > > > > > > > times
> > > > > > > > > create_emulated_cd and spice_usb_backend_device_unref and fails
> > > > > > > > > (https://gitlab.freedesktop.org/fziglio/spice-gtk/commit/0bbc0d85b43b5dbcb92c2a3915b4b1c9d9228a7a,
> > > > > > > > > will probably disappear in a while). I would expect this test
> > > > > > > > > to
> > > > > > > > > pass and to delete completely the object without leaks.
> > > > > > > >
> > > > > > > > This is wrong approach, I think.
> > > > > > > > The interface should be changed and shall not return the object.
> > > > > > > > (initial interface returned boolean)
> > > > > > > > usb-device-manager shall receive the device only via hotplug
> > > > > > > > indication, reference it and deref it when it receives unplug.
> > > > > > > >
> > > > > > >
> > > > > > > That make sense and be more coherent. In this case the call to
> > > > > > > spice_usb_backend_device_unref is surely wrong and should be moved
> > > > > > > to spice_usb_backend_create_emulated_device (as suggested below)
> > > > > > > to avoid leaks or use-after-free.
> > > > > > > I still think that a alloc/free test should work without calling
> > > > > > > a spice_usb_backend_device_eject function.
> > > > > >
> > > > > > This test is probably good for something that I do not see, but it
> > > > > > has
> > > > > > nothing common with how things work in reality.
> > > > > > The main difference with local usb devices (and the reason why teh
> > > > > > referencing is done differently) is that for local devices the list
> > > > > > of
> > > > > > existing devices is always maintained by somebody.
> > > > > > Local devices exist without any relation to backend object: in Linux
> > > > > > libusb maintains the list, in Windows libusb is able to return the
> > > > > > list.
> > > > > > I'd prefer to avoid this complication (i.e. maintaining list of all
> > > > > > the emulated devices).
> > > > > >
> > > > >
> > > > > I was not referring to an additional list, I was talking about
> > > > > "devices"
> > > > > array in SpiceUsbDeviceManagerPrivate. Devices (either from libusb and
> > > > > also emulated) are wrapped inside a SpiceUsbBackendDevice which is
> > > > > wrapped inside a SpiceUsbDevice and "devices" field keep all
> > > > > SpiceUsbDevice.
> > > > >
> > > > I just explain that the lifecycle of local devices and emulated
> > > > devices is different.
> > > > Let's take Linux as an example:
> > > > libusb maintains under libusb context the list of local devices (with
> > > > minimal refcount of 1).
> > > > When the device is just added: libusb calls hotplug indication, if the
> > > > usb-device-manager does not increment the refcount, nobody knows that
> > > > the device is exists, but it does.
> > > > (It will be reported, for example, if one registers hotplug callback -
> > > > hotplug callback will be fired with all existing devices from _this_
> > > > list).
> > > > Usually, of course, usb-device-manager references the backend device
> > > > and the backend device holds additional reference to the libusb
> > > > device.
> > > > When the local device disappears, hot unplug procedure happens, all
> > > > the guys (that used the device) dereference it, libusb also
> > > > dereferences it -> the device does not exist anymore.
> > > > With emulated devices we just use the fact that usb-device-manager
> > > > does not have any exceptions, it references each new device and
> > > > dereferences it when the device disappears.
> > > >
> > > > The problem of the test is:
> > > > it does create_cd_device, then unrefs it.and expect the device will be
> > > > freed correctly and completely.
> > > > But the unref is not an opposite of create_cd_device (for most of glib
> > > > object it seems natural, but this is not a glib object).
> > > > opposite of 'create' is 'eject'.
> > > >
> > > > Yes, we can make this test work, but _IMO_ this will not improve the
> > > > quality of the life to anybody (because the test uses the sequence of
> > > > operations that is not used in any real scenario).
> > > > For example, we can add backend * to the backend device and remove the
> > > > mask when the refcount of backend device goes to zero.
> > > > Is this what you want to be done?
> > > >
> > >
> > > What I want is code without evident leaks and with a coherent API and
> > > behaviour.
> > > Your code is currently leaking memory and the API is not clear breaking
> > > rules that are there and are correct.
> > I do not agree with this statement. The code does not have any leaks
> > in functional flows (AFAIK).
>
> As I said I instrumented code and also used some tools (like the address
> sanitizer) which are reporting the leak. Try to create a device with
> delete_on_eject set to 0. Or just close the program without ejecting
> the device (this should also happen migrating the machine but this
> was not tested).
> From the command line just not put "!" in front of the ISO image file name.
>
I agree, in this case there is the unfreed memory when the application exits.
As we are not taking about incremental leak (that happens each time
when something happens), this is IMO minor problem.
If you anyway want it to be solved - not a big problem, this requires
to maintain the list of created devices in backend object.
When the backend object is deleted, it will delete all the devices
that it has created.
> > The API does not break any existing rules (AFAIK). The rule 'unref' is
> > opposite for 'create' is not declared anywhere.
>
> If I see the name spice_usb_backend_create_emulated_device from the name
> I suppose this is creating a SpiceUsbBackendDevice object. If an
> object is created somebody has to release it otherwise you have a leak.
> Yes, the relationship between create and unref is not declared but the
> fact that a created object has to be released to avoid a leak I assume
> we agree on this.
Yes, on this we can agree (partially). The initiator of device
creation is usb-device-manager, but it can't decide when the device
shall disappear.
delete-on-eject was invented to demonstrate how the lifecycle of the
device could be similar to one of 'mounting' of CD-image.
IMO, this is for further discussion whether this should be
preferred/configurable mode of operation.
Alternative is to support load-unload-change-load concept, where the
device behave similarly to USB CD drive.
> The way usually an object with reference counting is released (and
> SpiceUsbBackendDevice has such feature) is usually calling a *_unref
> function or similar (and this is quite spread in spice-gtk and GLib,
> I hope we agree on this too).
>
This is not obvious. Indeed spice_usb_backend_create_emulated_device
requests to create the device
but the caller is not an owner of this device. It can't delete it any
time it wants.
For example, the GUI may have a function 'delete' the device, this
function shall request 'eject' and not just unref of what it has not
referenced.
Note that unref'ing redirected device does not stop the redirection,
when 'ejecting' does due to 'unplug' flow.
> > The test where the leak exists was invented after the patch was
> > submitted, from my personal point of view it is wrong (as uses
> > non-functional flows).
> > As I've state it is possible to make this test happy, if this is a
> > requirement.
>
> Fixing the test is not a requirement, removing the leak in the code
> yes.
>
> > >
> > > Watching your code something appeared not right on resource management
> > > and reference counting so I added some trace to understand and I quickly
> > > found that in different cases the resources were leaked. So I instrumented
> > > more, tracing the specific ref/unref and I found that emulated devices
> > > are different. The base rule of reference counting is that the counter
> > > represent the number of strong (owning) pointers, when there are no strong
> > > pointer the resource is freed. Maybe there are reasons (as you mentioned)
> > > to break this rule (I know that some implementations supporting weak
> > > references do) but the fact that a base rule is broken and that code
> > > has leaks suggests me that the code is wrong.
> > > In spice_usb_backend_device_eject you call spice_usb_backend_device_unref
> > > without a pointer change.
> > > For libusb devices yes, a list is maintained by libusb but libusb has
> > > nothing to do with SpiceUsbBackendDevice so I don't see why the
> > > reference counters of such objects (as you stated above) has to change
> > > between libusb and emulated devices.
> > > In hotplug_callback function a SpiceUsbBackendDevice object is created
> > > to wrap the libusb device but then after calling the hotplug_callback
> > > callback the reference is released as the ownership is passed to the
> > > callback. spice_usb_backend_create_emulated_device is not coherent in
> > > this, there no release (with or without my changes).
> > > So I tried to fix this rule breakage and remove the leak and I returned
> > > the SpiceUsbBackendDevice from spice_usb_backend_create_emulated_device
> > > (I could have called spice_usb_backend_device_unref and returned TRUE
> > > but decided to return the object and let the caller decide). As expected
> > > this created a use-after-free when spice_usb_backend_device_eject was
> > > called. Removing spice_usb_backend_device_unref from
> > > spice_usb_backend_device_eject fixed the use-after-free, the code with
> > > these changes has no leaks (setting or not delete_on_eject the
> > > SpiceUsbBackendDevice object is released).
> > > The test came after all these changes to check normal alloc+unref
> > > does not create leaks.
> > I do not exactly understant what you suggest.
> > If you have in mind some changes are mandatory, please refer them
> > specifically.
> > > Before your last series there were no code clearing own_devices_mask
> > > bits.
> > This was a mistake mentioned during review and fixed in the next round.
> >
> > > As these are set just before allocating a SpiceUsbBackendDevice
> > > I found it symmetric to clear them before object is fully destroyed
> > > so the test allocating 128 object and removing them.
> > > Then the code in spice_usb_backend_device_eject to clear these bits
> > > appeared in the next series without any notes, I suppose you realized
> > > the mistake. This is not symmetric but yes, I suppose the test could
> > > be adapted to this.
> > > The code is coming without much comments, try to see from the prospective
> > > of one person that does not know the code and has to change it.
> > The code does not include any significant changes from v1.
> > Is some lines of code from your POV require notes, please point on them.
> >
> > > Founding
> > > all code full of reference counting this person is expected *_unref
> > > functions to behave in a given way and to be called at specific
> > > conditions, breaking all these expectations with no comments in the
> > > code make the code unmaintainable and prone to bugs. Even more as
> > > there are no tests for the code added.
> > spice-gtk does not have any tests for usb redirection at all (except of
> > spicy).
> > I also was never asked for any tests for 'backend' series.
> > I have no idea what is your policy regarding the tests in general.
> > As I can see from existing repo or any other documentation, tests are
> > not required automatically for any new line of code.
> >
> > So, how exactly we are going to make progress?
> >
>
> Tests are fine and are a quality indications, as I said I wrote the
> test after founding the leak in the code and it's still WIP.
> The company is pushing us to write more tests but no, there's no
> requirement at the moment.
>
> I'll write a follow up of what's IMHO the right way to handle reference
> counting for emulated device (which will remove the leak)
>
> > >
> > > > > > Another option is just to change the test:
> > > > > > device = create_cd_device(backend, params)
> > > > > > spice_usb_backend_device_eject(backend, device)
> > > > > >
> > > > >
> > > > > In a previous mail you proposed to return a boolean and drop a
> > > > > reference so
> > > > > you won't have a pointer to device to call
> > > > > spice_usb_backend_device_eject.
> > > > >
> > > > > The test is supposed to use usb-backend APIs but
> > > > > spice_usb_backend_device_eject
> > > > > is not a usb-backend API so this would require that API to be
> > > > > moved/"promoted"
> > > > > to usb-backend. Also if this is the API this means that all
> > > > > create_cd_device
> > > > > should be followed by a spice_usb_backend_device_eject for consistency
> > > > > but
> > > > > this would cause the device to be remove straight away. Or you could
> > > > > add
> > > > > an additional list (that as you stated before you don't want) and call
> > > > > spice_usb_backend_device_eject at some other time (not clear when) but
> > > > > this
> > > > > should not be done automatically on SCSI state change as this would
> > > > > trigger
> > > > > a use-after-free as the reference counter get negative.
> > > > > Or you would have to add a rule that you need to call
> > > > > spice_usb_backend_device_eject only if you don't set delete_on_eject.
> > > > > Honestly these rules looks to me weird and complicated.
> > > > > Resource management usually has some simple rules like "if you allocate
> > > > > with malloc release with free" or "if you need to keep a reference
> > > > > call XXX_ref, if you need to remove a reference call XXX_unref",
> > > > > quite far from the APIs you are proposing.
> > > > > My test did some simple assumption, "if you get a reference to an
> > > > > emulated device you can remove it with spice_usb_backend_device_unref",
> > > > > as the emulated device is a SpiceUsbBackendDevice it seems logical
> > > > > to me.
> > > > >
> > > > > It seems more consistent to me if either the "devices" list holds the
> > > > > "main" (say initial) reference to the emulated device so in case
> > > > > spice_usb_backend_device_eject is called by the SCSI layer all it has
> > > > > to do is calling the hotplug function to remove from this list.
> > > > > For this reason the call to spice_usb_backend_device_unref in
> > > > > spice_usb_backend_device_eject seems wrong to me.
> > > > > If the device is not released by spice_usb_backend_device_eject will
> > > > > be released when the usb device manager release "devices".
> > > > > The only problem (currently) is that own_devices_mask bit is not
> > > > > cleared
> > > > > (not that is impossible to do it). I also would consider corrent to
> > > > > clear own_devices_mask bit when the device is released in order to
> > > > > avoid to have two emulated devices with the same address at the same
> > > > > time.
> > > > >
> > > > > > >
> > > > > > > > >
> > > > > > > > > > +}
> > > > > > > > > > +
> > > > > > > > > > +SpiceUsbBackendDevice*
> > > > > > > > > > +spice_usb_backend_create_emulated_device(SpiceUsbBackend
> > > > > > > > > > *be,
> > > > > > > > > > +
> > > > > > > > > > SpiceUsbEmulatedDeviceCreate
> > > > > > > > > > create_proc,
> > > > > > > > > > + void
> > > > > > > > > > *create_params,
> > > > > > > > > > + GError **err)
> > > > > > > > > > +{
> > > > > > > > > > + SpiceUsbEmulatedDevice *edev;
> > > > > > > > > > + SpiceUsbBackendDevice *dev;
> > > > > > > > > > + struct libusb_device_descriptor *desc;
> > > > > > > > > > + uint16_t device_desc_size;
> > > > > > > > > > + uint8_t address = 0;
> > > > > > > > > > +
> > > > > > > > > > + if (be->own_devices_mask == 0xffffffff) {
> > > > > > > > > > + g_set_error(err, SPICE_CLIENT_ERROR,
> > > > > > > > > > SPICE_CLIENT_ERROR_FAILED,
> > > > > > > > > > + _("can't create device - limit
> > > > > > > > > > reached"));
> > > > > > > > > > + return NULL;
> > > > > > > > > > + }
> > > > > > > > > > + for (address = 0; address < 32; ++address) {
> > > > > > > > > > + if (~be->own_devices_mask & (1 << address)) {
> > > > > > > > > > + break;
> > > > > > > > > > + }
> > > > > > > > > > + }
> > > > > > > > > > +
> > > > > > > > > > + dev = g_new0(SpiceUsbBackendDevice, 1);
> > > > > > > > > > + dev->device_info.bus = BUS_NUMBER_FOR_EMULATED_USB;
> > > > > > > > > > + dev->device_info.address = address;
> > > > > > > > > > + dev->ref_count = 1;
> > > > > > > > > > +
> > > > > > > > > > + dev->edev = edev = create_proc(be, dev, create_params,
> > > > > > > > > > err);
> > > > > > > > > > + if (edev == NULL) {
> > > > > > > > > > + spice_usb_backend_device_unref(dev);
> > > > > > > > > > + return NULL;
> > > > > > > > > > + }
> > > > > > > > > > +
> > > > > > > > > > + if (!device_ops(edev)->get_descriptor(edev,
> > > > > > > > > > LIBUSB_DT_DEVICE, 0,
> > > > > > > > > > + (void **)&desc,
> > > > > > > > > > &device_desc_size)
> > > > > > > > > > + || device_desc_size != sizeof(*desc)) {
> > > > > > > > > > +
> > > > > > > > > > + spice_usb_backend_device_unref(dev);
> > > > > > > > > > + g_set_error(err, SPICE_CLIENT_ERROR,
> > > > > > > > > > SPICE_CLIENT_ERROR_FAILED,
> > > > > > > > > > + _("can't create device - internal
> > > > > > > > > > error"));
> > > > > > > > > > + return NULL;
> > > > > > > > > > + }
> > > > > > > > > > +
> > > > > > > > > > + be->own_devices_mask |= 1 << address;
> > > > > > > > > > +
> > > > > > > > > > + dev->device_info.vid = desc->idVendor;
> > > > > > > > > > + dev->device_info.pid = desc->idProduct;
> > > > > > > > > > + dev->device_info.bcdUSB = desc->bcdUSB;
> > > > > > > > > > + dev->device_info.class = desc->bDeviceClass;
> > > > > > > > > > + dev->device_info.subclass = desc->bDeviceSubClass;
> > > > > > > > > > + dev->device_info.protocol = desc->bDeviceProtocol;
> > > > > > > > > > +
> > > > > > > > > > + if (be->hotplug_callback) {
> > > > > > > > > > + be->hotplug_callback(be->hotplug_user_data, dev,
> > > > > > > > > > TRUE);
> > > > > > > > > > + }
> > > > > > > > >
> > > > > > > > > Here the difference from normal devices. In normal devices
> > > > > > > > > hotplug_callback callback is called from hotplug_callback
> > > > > > > > > function
> > > > > > > > > then
> > > > > > > > > device is released with spice_usb_backend_device_unref. Here
> > > > > > > > > the
> > > > > > > > > function returns the object. This pointer is returned by
> > > > > > > > > create_emulated_cd
> > > > > > > > > then the caller (spice_usb_device_manager_set_property) just
> > > > > > > > > discard
> > > > > > > > > the pointer.
> > > > > > > > >
> > > > > > > > > > +
> > > > > > > > > > + return dev;
> > > > > > > > > > +}
> > > > > > > > > > +
> > > > > > > > > > #endif /* USB_REDIR */
> > > > > > > > > > diff --git a/src/usb-backend.h b/src/usb-backend.h
> > > > > > > > > > index 69a490b..63b9202 100644
> > > > > > > > > > --- a/src/usb-backend.h
> > > > > > > > > > +++ b/src/usb-backend.h
> > > > > > > > > > @@ -31,12 +31,15 @@ typedef struct _SpiceUsbBackend
> > > > > > > > > > SpiceUsbBackend;
> > > > > > > > > > typedef struct _SpiceUsbBackendDevice SpiceUsbBackendDevice;
> > > > > > > > > > typedef struct _SpiceUsbBackendChannel
> > > > > > > > > > SpiceUsbBackendChannel;
> > > > > > > > > >
> > > > > > > > > > +#define BUS_NUMBER_FOR_EMULATED_USB G_MAXUINT16
> > > > > > > > > > +
> > > > > > > > > > typedef struct UsbDeviceInformation
> > > > > > > > > > {
> > > > > > > > > > uint16_t bus;
> > > > > > > > > > uint16_t address;
> > > > > > > > > > uint16_t vid;
> > > > > > > > > > uint16_t pid;
> > > > > > > > > > + uint16_t bcdUSB;
> > > > > > > > > > uint8_t class;
> > > > > > > > > > uint8_t subclass;
> > > > > > > > > > uint8_t protocol;
> > > > > > > > > > diff --git a/src/usb-emulation.h b/src/usb-emulation.h
> > > > > > > > > > new file mode 100644
> > > > > > > > > > index 0000000..9e626a2
> > > > > > > > > > --- /dev/null
> > > > > > > > > > +++ b/src/usb-emulation.h
> > > > > > > > > > @@ -0,0 +1,91 @@
> > > > > > > > > > +/* -*- Mode: C; c-basic-offset: 4; indent-tabs-mode: nil -*-
> > > > > > > > > > */
> > > > > > > > > > +/*
> > > > > > > > > > + Copyright (C) 2019 Red Hat, Inc.
> > > > > > > > > > +
> > > > > > > > > > + Red Hat Authors:
> > > > > > > > > > + Yuri Benditovich<ybendito at redhat.com>
> > > > > > > > > > +
> > > > > > > > > > + This library is free software; you can redistribute it
> > > > > > > > > > and/or
> > > > > > > > > > + modify it under the terms of the GNU Lesser General
> > > > > > > > > > Public
> > > > > > > > > > + License as published by the Free Software Foundation;
> > > > > > > > > > either
> > > > > > > > > > + version 2.1 of the License, or (at your option) any
> > > > > > > > > > later
> > > > > > > > > > version.
> > > > > > > > > > +
> > > > > > > > > > + This library is distributed in the hope that it will be
> > > > > > > > > > useful,
> > > > > > > > > > + but WITHOUT ANY WARRANTY; without even the implied
> > > > > > > > > > warranty
> > > > > > > > > > of
> > > > > > > > > > + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
> > > > > > > > > > See
> > > > > > > > > > the
> > > > > > > > > > GNU
> > > > > > > > > > + Lesser General Public License for more details.
> > > > > > > > > > +
> > > > > > > > > > + You should have received a copy of the GNU Lesser
> > > > > > > > > > General
> > > > > > > > > > Public
> > > > > > > > > > + License along with this library; if not, see
> > > > > > > > > > <http://www.gnu.org/licenses/>.
> > > > > > > > > > +*/
> > > > > > > > > > +
> > > > > > > > > > +#ifndef __SPICE_USB_EMULATION_H__
> > > > > > > > > > +#define __SPICE_USB_EMULATION_H__
> > > > > > > > > > +
> > > > > > > > > > +#include "usbredirparser.h"
> > > > > > > > > > +#include "usb-backend.h"
> > > > > > > > > > +
> > > > > > > > > > +typedef struct SpiceUsbEmulatedDevice
> > > > > > > > > > SpiceUsbEmulatedDevice;
> > > > > > > > > > +typedef SpiceUsbEmulatedDevice*
> > > > > > > > > > +(*SpiceUsbEmulatedDeviceCreate)(SpiceUsbBackend *be,
> > > > > > > > > > + SpiceUsbBackendDevice
> > > > > > > > > > *parent,
> > > > > > > > > > + void *create_params,
> > > > > > > > > > + GError **err);
> > > > > > > > > > +
> > > > > > > > > > +/*
> > > > > > > > > > + function table for emulated USB device
> > > > > > > > > > + must be first member of device structure
> > > > > > > > > > + all functions are mandatory for implementation
> > > > > > > > > > +*/
> > > > > > > > > > +typedef struct UsbDeviceOps {
> > > > > > > > > > + gboolean (*get_descriptor)(SpiceUsbEmulatedDevice
> > > > > > > > > > *device,
> > > > > > > > > > + uint8_t type, uint8_t index,
> > > > > > > > > > + void **buffer, uint16_t
> > > > > > > > > > *size);
> > > > > > > > > > + gchar *
> > > > > > > > > > (*get_product_description)(SpiceUsbEmulatedDevice
> > > > > > > > > > *device);
> > > > > > > > > > + void (*attach)(SpiceUsbEmulatedDevice *device, struct
> > > > > > > > > > usbredirparser
> > > > > > > > > > *parser);
> > > > > > > > > > + void (*reset)(SpiceUsbEmulatedDevice *device);
> > > > > > > > > > + /*
> > > > > > > > > > + processing is synchronous, default = stall:
> > > > > > > > > > + - return success without data: set status to 0
> > > > > > > > > > + - return error - set status to error
> > > > > > > > > > + - return success with data - set status to 0,
> > > > > > > > > > + set buffer to some
> > > > > > > > > > buffer
> > > > > > > > > > + set length to out len
> > > > > > > > > > + truncation is automatic
> > > > > > > > > > + */
> > > > > > > > > > + void (*control_request)(SpiceUsbEmulatedDevice *device,
> > > > > > > > > > + uint8_t *data, int data_len,
> > > > > > > > > > + struct
> > > > > > > > > > usb_redir_control_packet_header
> > > > > > > > > > *h,
> > > > > > > > > > + void **buffer);
> > > > > > > > > > + /*
> > > > > > > > > > + processing is synchronous:
> > > > > > > > > > + - set h->status to resulting status, default = stall
> > > > > > > > > > + */
> > > > > > > > > > + void (*bulk_out_request)(SpiceUsbEmulatedDevice *device,
> > > > > > > > > > + uint8_t ep, uint8_t *data, int
> > > > > > > > > > data_len,
> > > > > > > > > > + uint8_t *status);
> > > > > > > > > > + /*
> > > > > > > > > > + if returns true, processing is asynchronous
> > > > > > > > > > + otherwise header contains error status
> > > > > > > > > > + */
> > > > > > > > > > + gboolean (*bulk_in_request)(SpiceUsbEmulatedDevice
> > > > > > > > > > *device,
> > > > > > > > > > uint64_t
> > > > > > > > > > id,
> > > > > > > > > > + struct
> > > > > > > > > > usb_redir_bulk_packet_header
> > > > > > > > > > *bulk_header);
> > > > > > > > > > + void (*cancel_request)(SpiceUsbEmulatedDevice *device,
> > > > > > > > > > uint64_t
> > > > > > > > > > id);
> > > > > > > > > > + void (*detach)(SpiceUsbEmulatedDevice *device);
> > > > > > > > > > + void (*unrealize)(SpiceUsbEmulatedDevice *device);
> > > > > > > > > > +} UsbDeviceOps;
> > > > > > > > > > +
> > > > > > > > > > +static inline const UsbDeviceOps
> > > > > > > > > > *device_ops(SpiceUsbEmulatedDevice
> > > > > > > > > > *dev)
> > > > > > > > > > +{
> > > > > > > > > > + return (const UsbDeviceOps *)dev;
> > > > > > > > > > +}
> > > > > > > > > > +
> > > > > > > > > > +SpiceUsbBackendDevice*
> > > > > > > > > > +spice_usb_backend_create_emulated_device(SpiceUsbBackend
> > > > > > > > > > *be,
> > > > > > > > > > +
> > > > > > > > > > SpiceUsbEmulatedDeviceCreate
> > > > > > > > > > create_proc,
> > > > > > > > > > + void
> > > > > > > > > > *create_params,
> > > > > > > > > > + GError **err);
> > > > > > > > > > +void spice_usb_backend_device_eject(SpiceUsbBackend *be,
> > > > > > > > > > SpiceUsbBackendDevice *device);
> > > > > > > > > > +void spice_usb_backend_device_report_change(SpiceUsbBackend
> > > > > > > > > > *be,
> > > > > > > > > > SpiceUsbBackendDevice *device);
> > > > > > > > > > +
> > > > > > > > > > +#endif
> > > > > > > > >
> > > > > > > > > Frediano
> > > > > > > >
> > > > > >
> > > >
> >
More information about the Spice-devel
mailing list