[spice-gtk,v4,26/29] test-cd-emu: Test attach/detach emulated device

Submitted by Frediano Ziglio on Aug. 27, 2019, 9:22 a.m.

Details

Message ID 20190827092246.10276-27-fziglio@redhat.com
State Superseded
Headers show
Series "added feature of sharing CD image" ( rev: 6 ) in Spice

Not browsing as part of any series.

Commit Message

Frediano Ziglio Aug. 27, 2019, 9:22 a.m.
Mock some usb-backend functions to be able to simulate device
attachment and detachment.
Create session and channel to pass some valid pointer anyway.
Emulate channel state correctly.
Make sure HELLO packets are sent correctly at the beginning and
no more afterwards.
Test auto-connect enabled or disabled.

Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
---
 tests/cd-emu.c | 153 ++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 151 insertions(+), 2 deletions(-)

Patch hide | download patch | download mbox

diff --git a/tests/cd-emu.c b/tests/cd-emu.c
index 7bf1fa3c..f0966662 100644
--- a/tests/cd-emu.c
+++ b/tests/cd-emu.c
@@ -14,10 +14,13 @@ 
    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/>.
 */
-#include <gio/gio.h>
+
+// Mock some code in usb-backend.c
+#define spice_usbredir_write_callback mock_spice_usbredir_write_callback
+#define spice_channel_get_state mock_spice_channel_get_state
+#include "../src/usb-backend.c"
 
 #include "usb-device-cd.h"
-#include "usb-emulation.h"
 
 static SpiceUsbBackendDevice *device = NULL;
 
@@ -67,6 +70,150 @@  static void multiple(const void *param)
     spice_usb_backend_delete(be);
 }
 
+static unsigned int messages_sent = 0;
+static unsigned int hellos_sent = 0;
+static SpiceUsbBackendChannel *usb_ch;
+
+int
+mock_spice_usbredir_write_callback(SpiceUsbredirChannel *channel, uint8_t *data, int count)
+{
+    ++messages_sent;
+    g_assert_cmpint(count, >=, 4);
+    const uint32_t type = data[0] + data[1] * 0x100u + data[2] * 0x10000u + data[3] * 0x1000000u;
+    if (type == usb_redir_hello) {
+        ++hellos_sent;
+    }
+
+    // return we handled the data
+    spice_usb_backend_return_write_data(usb_ch, data);
+    return count;
+}
+
+// channel state to return from Mock function
+static enum spice_channel_state ch_state = SPICE_CHANNEL_STATE_UNCONNECTED;
+
+enum spice_channel_state
+mock_spice_channel_get_state(SpiceChannel *channel)
+{
+    return ch_state;
+}
+
+// number of GObjects allocated we expect will be freed
+static unsigned gobjects_allocated = 0;
+static void decrement_allocated(gpointer data G_GNUC_UNUSED, GObject *old_gobject G_GNUC_UNUSED)
+{
+    g_assert_cmpint(gobjects_allocated, !=, 0);
+    --gobjects_allocated;
+}
+
+#define DATA_START \
+    do { static const uint8_t data[] = {
+#define DATA_SEND \
+        }; \
+        spice_usb_backend_read_guest_data(usb_ch, (uint8_t*)data, G_N_ELEMENTS(data)); \
+    } while(0)
+
+static void attach(const void *param)
+{
+    const bool attach_on_connect = !!GPOINTER_TO_UINT(param);
+
+    hellos_sent = 0;
+    messages_sent = 0;
+    ch_state = SPICE_CHANNEL_STATE_UNCONNECTED;
+
+    SpiceSession *session = spice_session_new();
+    g_assert_nonnull(session);
+    g_object_weak_ref(G_OBJECT(session), decrement_allocated, NULL);
+    SpiceChannel *ch = spice_channel_new(session, SPICE_CHANNEL_USBREDIR, 0);
+    g_assert_nonnull(ch);
+    g_object_weak_ref(G_OBJECT(ch), decrement_allocated, NULL);
+    gobjects_allocated = 2;
+
+    /*
+     * real test, allocate a channel usbredir, emulate device
+     * initialization.
+     * Filter some call.
+     * Start sequence:
+     * - spice_usb_backend_new
+     * - spice_usb_backend_register_hotplug
+     * - spice_usb_backend_create_emulated_device
+     * - spice_usb_backend_channel_new
+     * - spice_usb_backend_channel_attach (if redir on connect)
+     * - spice_usb_backend_channel_flush_writes
+     * - spice_usbredir_write_callback
+     * - spice_usb_backend_return_write_data
+     * - spice_usb_backend_read_guest_data
+     * - spice_usb_backend_channel_attach (if not redir on connect)
+     */
+    GError *err = NULL;
+    SpiceUsbBackend * be = spice_usb_backend_new(&err);
+    g_assert_nonnull(be);
+    g_assert_null(err);
+    spice_usb_backend_register_hotplug(be, NULL, test_hotplug_callback, &err);
+    g_assert_null(err);
+
+    CdEmulationParams params = { "test-cd-emu.iso", 1 };
+    g_assert_true(create_emulated_cd(be, &params, &err));
+    g_assert_null(err);
+    g_assert_nonnull(device);
+
+    usb_ch = spice_usb_backend_channel_new(be, SPICE_USBREDIR_CHANNEL(ch));
+    g_assert_nonnull(usb_ch);
+
+    // attach on connect
+    ch_state = SPICE_CHANNEL_STATE_CONNECTING;
+    if (attach_on_connect) {
+        g_assert_true(spice_usb_backend_channel_attach(usb_ch, device, &err));
+        g_assert_null(err);
+    }
+    g_assert_cmpint(hellos_sent, ==, 0);
+    g_assert_cmpint(messages_sent, ==, 0);
+
+    // try to get initial data
+    ch_state = SPICE_CHANNEL_STATE_READY;
+    spice_usb_backend_channel_flush_writes(usb_ch);
+
+    // we should get an hello (only one!)
+    g_assert_cmpint(hellos_sent, ==, 1);
+    g_assert_cmpint(messages_sent, ==, 1);
+
+    // send hello reply
+    DATA_START
+        0x00,0x00,0x00,0x00,0x44,0x00,0x00,0x00,0x00,0x00,0x00,0x00, //000 ....D.......
+        0x71,0x65,0x6d,0x75,0x20,0x75,0x73,0x62,0x2d,0x72,0x65,0x64, //00c qemu usb-red
+        0x69,0x72,0x20,0x67,0x75,0x65,0x73,0x74,0x20,0x33,0x2e,0x30, //018 ir guest 3.0
+        0x2e,0x31,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00, //024 .1..........
+        0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00, //030 ............
+        0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00, //03c ............
+        0x00,0x00,0x00,0x00,0xff,0x00,0x00,0x00,                     //048 ........
+    DATA_SEND;
+
+    if (!attach_on_connect) {
+        g_assert_true(spice_usb_backend_channel_attach(usb_ch, device, &err));
+        g_assert_null(err);
+    }
+    g_assert_cmpint(hellos_sent, ==, 1);
+    g_assert_cmpint(messages_sent, >, 1);
+
+    // cleanup
+    spice_usb_backend_device_unref(device);
+    device = NULL;
+    spice_usb_backend_channel_delete(usb_ch);
+    usb_ch = NULL;
+    spice_usb_backend_deregister_hotplug(be);
+    spice_usb_backend_delete(be);
+
+    // this it's the correct sequence to free session!
+    // g_object_unref is not enough, causing wrong reference countings
+    spice_session_disconnect(session);
+    g_object_unref(session);
+    while (g_main_context_iteration(NULL, FALSE)) {
+        continue;
+    }
+
+    g_assert_cmpint(gobjects_allocated, ==, 0);
+}
+
 static void
 write_test_iso(void)
 {
@@ -87,6 +234,8 @@  int main(int argc, char* argv[])
 
     g_test_add_data_func("/cd-emu/simple", GUINT_TO_POINTER(1), multiple);
     g_test_add_data_func("/cd-emu/multiple", GUINT_TO_POINTER(128), multiple);
+    g_test_add_data_func("/cd-emu/attach_no_auto", GUINT_TO_POINTER(0), attach);
+    g_test_add_data_func("/cd-emu/attach_auto", GUINT_TO_POINTER(1), attach);
 
     return g_test_run();
 }

Comments

Frediano Ziglio writes:

> Mock some usb-backend functions to be able to simulate device
> attachment and detachment.
> +
> +    // this it's the correct sequence to free session!
> +    // g_object_unref is not enough, causing wrong reference countings
> +    spice_session_disconnect(session);
> +    g_object_unref(session);
> +    while (g_main_context_iteration(NULL, FALSE)) {
> +        continue;
> +    }

This looks so peculiar that it might be worth putting in its
own function, in order to facilitate reuse.

> +
> +    g_assert_cmpint(gobjects_allocated, ==, 0);
> +}
> +
>  static void
>  write_test_iso(void)
>  {
> @@ -87,6 +234,8 @@ int main(int argc, char* argv[])
>
>      g_test_add_data_func("/cd-emu/simple", GUINT_TO_POINTER(1), multiple);
>      g_test_add_data_func("/cd-emu/multiple", GUINT_TO_POINTER(128), multiple);
> +    g_test_add_data_func("/cd-emu/attach_no_auto", GUINT_TO_POINTER(0), attach);
> +    g_test_add_data_func("/cd-emu/attach_auto", GUINT_TO_POINTER(1), attach);
>
>      return g_test_run();
>  }


--
Cheers,
Christophe de Dinechin (IRC c3d)
> 
> Frediano Ziglio writes:
> 
> > Mock some usb-backend functions to be able to simulate device
> > attachment and detachment.
> > +
> > +    // this it's the correct sequence to free session!
> > +    // g_object_unref is not enough, causing wrong reference countings
> > +    spice_session_disconnect(session);
> > +    g_object_unref(session);
> > +    while (g_main_context_iteration(NULL, FALSE)) {
> > +        continue;
> > +    }
> 
> This looks so peculiar that it might be worth putting in its
> own function, in order to facilitate reuse.
> 

I was thinking about a DoEvents function in memory of the good old VB!
(well, do_events in C).
Getting these lines right was not really easy! Calling g_object_unref
directly causes multiple crashes. The deallocations are queued in GLib.

> > +
> > +    g_assert_cmpint(gobjects_allocated, ==, 0);
> > +}
> > +
> >  static void
> >  write_test_iso(void)
> >  {
> > @@ -87,6 +234,8 @@ int main(int argc, char* argv[])
> >
> >      g_test_add_data_func("/cd-emu/simple", GUINT_TO_POINTER(1), multiple);
> >      g_test_add_data_func("/cd-emu/multiple", GUINT_TO_POINTER(128),
> >      multiple);
> > +    g_test_add_data_func("/cd-emu/attach_no_auto", GUINT_TO_POINTER(0),
> > attach);
> > +    g_test_add_data_func("/cd-emu/attach_auto", GUINT_TO_POINTER(1),
> > attach);
> >
> >      return g_test_run();
> >  }
> 

Frediano
Frediano Ziglio writes:

>>
>> Frediano Ziglio writes:
>>
>> > Mock some usb-backend functions to be able to simulate device
>> > attachment and detachment.
>> > +
>> > +    // this it's the correct sequence to free session!
>> > +    // g_object_unref is not enough, causing wrong reference countings
>> > +    spice_session_disconnect(session);
>> > +    g_object_unref(session);
>> > +    while (g_main_context_iteration(NULL, FALSE)) {
>> > +        continue;
>> > +    }
>>
>> This looks so peculiar that it might be worth putting in its
>> own function, in order to facilitate reuse.
>>
>
> I was thinking about a DoEvents function in memory of the good old VB!
> (well, do_events in C).
> Getting these lines right was not really easy! Calling g_object_unref
> directly causes multiple crashes. The deallocations are queued in GLib.

Is the problem specific to this test? If so, might be worth explaining
why, otherwise someone might break it in the future.

>
>> > +
>> > +    g_assert_cmpint(gobjects_allocated, ==, 0);
>> > +}
>> > +
>> >  static void
>> >  write_test_iso(void)
>> >  {
>> > @@ -87,6 +234,8 @@ int main(int argc, char* argv[])
>> >
>> >      g_test_add_data_func("/cd-emu/simple", GUINT_TO_POINTER(1), multiple);
>> >      g_test_add_data_func("/cd-emu/multiple", GUINT_TO_POINTER(128),
>> >      multiple);
>> > +    g_test_add_data_func("/cd-emu/attach_no_auto", GUINT_TO_POINTER(0),
>> > attach);
>> > +    g_test_add_data_func("/cd-emu/attach_auto", GUINT_TO_POINTER(1),
>> > attach);
>> >
>> >      return g_test_run();
>> >  }
>>
>
> Frediano


--
Cheers,
Christophe de Dinechin (IRC c3d)

> 
> Hi,
> 
> On Tue, Aug 27, 2019 at 10:22:43AM +0100, Frediano Ziglio wrote:
> > Mock some usb-backend functions to be able to simulate device
> > attachment and detachment.
> > Create session and channel to pass some valid pointer anyway.
> > Emulate channel state correctly.
> > Make sure HELLO packets are sent correctly at the beginning and
> > no more afterwards.
> > Test auto-connect enabled or disabled.
> > 
> > Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
> > ---
> >  tests/cd-emu.c | 153 ++++++++++++++++++++++++++++++++++++++++++++++++-
> >  1 file changed, 151 insertions(+), 2 deletions(-)
> > 
> > diff --git a/tests/cd-emu.c b/tests/cd-emu.c
> > index 7bf1fa3c..f0966662 100644
> > --- a/tests/cd-emu.c
> > +++ b/tests/cd-emu.c
> > @@ -14,10 +14,13 @@
> >     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/>.
> >  */
> > -#include <gio/gio.h>
> > +
> > +// Mock some code in usb-backend.c
> > +#define spice_usbredir_write_callback mock_spice_usbredir_write_callback
> > +#define spice_channel_get_state mock_spice_channel_get_state
> 
> I wonder if isn't better to create a -private header for this
> instead?
> 

That would solve calling static functions but not allow mocking functions
which is the main purpose here.
Yes, it seems hacky but from the solutions I've found it's one of the less
hacky, really.
Also it does not change the tested code at all and it does not require to
compile the source with multiple options which is nice.

> > +#include "../src/usb-backend.c"
> >  
> >  #include "usb-device-cd.h"
> > -#include "usb-emulation.h"
> >  
> >  static SpiceUsbBackendDevice *device = NULL;
> >  
> > @@ -67,6 +70,150 @@ static void multiple(const void *param)
> >      spice_usb_backend_delete(be);
> >  }
> >  
> > +static unsigned int messages_sent = 0;
> > +static unsigned int hellos_sent = 0;
> > +static SpiceUsbBackendChannel *usb_ch;
> > +
> > +int
> > +mock_spice_usbredir_write_callback(SpiceUsbredirChannel *channel, uint8_t
> > *data, int count)
> > +{
> > +    ++messages_sent;
> > +    g_assert_cmpint(count, >=, 4);
> > +    const uint32_t type = data[0] + data[1] * 0x100u + data[2] * 0x10000u
> > + data[3] * 0x1000000u;
> > +    if (type == usb_redir_hello) {
> > +        ++hellos_sent;
> > +    }
> > +
> > +    // return we handled the data
> > +    spice_usb_backend_return_write_data(usb_ch, data);
> > +    return count;
> > +}
> > +
> > +// channel state to return from Mock function
> > +static enum spice_channel_state ch_state =
> > SPICE_CHANNEL_STATE_UNCONNECTED;
> > +
> > +enum spice_channel_state
> > +mock_spice_channel_get_state(SpiceChannel *channel)
> > +{
> > +    return ch_state;
> > +}
> > +
> > +// number of GObjects allocated we expect will be freed
> > +static unsigned gobjects_allocated = 0;
> > +static void decrement_allocated(gpointer data G_GNUC_UNUSED, GObject
> > *old_gobject G_GNUC_UNUSED)
> > +{
> > +    g_assert_cmpint(gobjects_allocated, !=, 0);
> > +    --gobjects_allocated;
> > +}
> > +
> > +#define DATA_START \
> > +    do { static const uint8_t data[] = {
> > +#define DATA_SEND \
> > +        }; \
> > +        spice_usb_backend_read_guest_data(usb_ch, (uint8_t*)data,
> > G_N_ELEMENTS(data)); \
> > +    } while(0)
> > +
> > +static void attach(const void *param)
> > +{
> > +    const bool attach_on_connect = !!GPOINTER_TO_UINT(param);
> > +
> > +    hellos_sent = 0;
> > +    messages_sent = 0;
> > +    ch_state = SPICE_CHANNEL_STATE_UNCONNECTED;
> > +
> > +    SpiceSession *session = spice_session_new();
> > +    g_assert_nonnull(session);
> > +    g_object_weak_ref(G_OBJECT(session), decrement_allocated, NULL);
> > +    SpiceChannel *ch = spice_channel_new(session, SPICE_CHANNEL_USBREDIR,
> > 0);
> > +    g_assert_nonnull(ch);
> > +    g_object_weak_ref(G_OBJECT(ch), decrement_allocated, NULL);
> > +    gobjects_allocated = 2;
> > +
> > +    /*
> > +     * real test, allocate a channel usbredir, emulate device
> > +     * initialization.
> > +     * Filter some call.
> > +     * Start sequence:
> > +     * - spice_usb_backend_new
> > +     * - spice_usb_backend_register_hotplug
> > +     * - spice_usb_backend_create_emulated_device
> > +     * - spice_usb_backend_channel_new
> > +     * - spice_usb_backend_channel_attach (if redir on connect)
> > +     * - spice_usb_backend_channel_flush_writes
> > +     * - spice_usbredir_write_callback
> > +     * - spice_usb_backend_return_write_data
> > +     * - spice_usb_backend_read_guest_data
> > +     * - spice_usb_backend_channel_attach (if not redir on connect)
> > +     */
> > +    GError *err = NULL;
> > +    SpiceUsbBackend * be = spice_usb_backend_new(&err);
> > +    g_assert_nonnull(be);
> > +    g_assert_null(err);
> > +    spice_usb_backend_register_hotplug(be, NULL, test_hotplug_callback,
> > &err);
> > +    g_assert_null(err);
> > +
> > +    CdEmulationParams params = { "test-cd-emu.iso", 1 };
> > +    g_assert_true(create_emulated_cd(be, &params, &err));
> > +    g_assert_null(err);
> > +    g_assert_nonnull(device);
> > +
> > +    usb_ch = spice_usb_backend_channel_new(be,
> > SPICE_USBREDIR_CHANNEL(ch));
> > +    g_assert_nonnull(usb_ch);
> > +
> > +    // attach on connect
> > +    ch_state = SPICE_CHANNEL_STATE_CONNECTING;
> > +    if (attach_on_connect) {
> > +        g_assert_true(spice_usb_backend_channel_attach(usb_ch, device,
> > &err));
> > +        g_assert_null(err);
> > +    }
> > +    g_assert_cmpint(hellos_sent, ==, 0);
> > +    g_assert_cmpint(messages_sent, ==, 0);
> > +
> > +    // try to get initial data
> > +    ch_state = SPICE_CHANNEL_STATE_READY;
> > +    spice_usb_backend_channel_flush_writes(usb_ch);
> > +
> > +    // we should get an hello (only one!)
> > +    g_assert_cmpint(hellos_sent, ==, 1);
> > +    g_assert_cmpint(messages_sent, ==, 1);
> > +
> > +    // send hello reply
> > +    DATA_START
> > +        0x00,0x00,0x00,0x00,0x44,0x00,0x00,0x00,0x00,0x00,0x00,0x00, //000
> > ....D.......
> > +        0x71,0x65,0x6d,0x75,0x20,0x75,0x73,0x62,0x2d,0x72,0x65,0x64, //00c
> > qemu usb-red
> > +        0x69,0x72,0x20,0x67,0x75,0x65,0x73,0x74,0x20,0x33,0x2e,0x30, //018
> > ir guest 3.0
> > +        0x2e,0x31,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00, //024
> > .1..........
> > +        0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00, //030
> > ............
> > +        0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00, //03c
> > ............
> > +        0x00,0x00,0x00,0x00,0xff,0x00,0x00,0x00,                     //048
> > ........
> > +    DATA_SEND;
> 
> Should we have different 'hellos' in the future? Let's say we
> support 3.0 but not 3.1, etc.
> 

That "3.0" is not much important, it's just a string you can write "Pizza Margherita"
and code will be still happy. What's worth are the last 4 bytes, but that we can change
if we need in the future.

> > +
> > +    if (!attach_on_connect) {
> > +        g_assert_true(spice_usb_backend_channel_attach(usb_ch, device,
> > &err));
> > +        g_assert_null(err);
> > +    }
> > +    g_assert_cmpint(hellos_sent, ==, 1);
> > +    g_assert_cmpint(messages_sent, >, 1);
> > +
> > +    // cleanup
> > +    spice_usb_backend_device_unref(device);
> > +    device = NULL;
> > +    spice_usb_backend_channel_delete(usb_ch);
> > +    usb_ch = NULL;
> > +    spice_usb_backend_deregister_hotplug(be);
> > +    spice_usb_backend_delete(be);
> > +
> > +    // this it's the correct sequence to free session!
> > +    // g_object_unref is not enough, causing wrong reference countings
> > +    spice_session_disconnect(session);
> > +    g_object_unref(session);
> > +    while (g_main_context_iteration(NULL, FALSE)) {
> > +        continue;
> > +    }
> > +
> > +    g_assert_cmpint(gobjects_allocated, ==, 0);
> > +}
> > +
> >  static void
> >  write_test_iso(void)
> >  {
> > @@ -87,6 +234,8 @@ int main(int argc, char* argv[])
> >  
> >      g_test_add_data_func("/cd-emu/simple", GUINT_TO_POINTER(1), multiple);
> >      g_test_add_data_func("/cd-emu/multiple", GUINT_TO_POINTER(128),
> >      multiple);
> > +    g_test_add_data_func("/cd-emu/attach_no_auto", GUINT_TO_POINTER(0),
> > attach);
> > +    g_test_add_data_func("/cd-emu/attach_auto", GUINT_TO_POINTER(1),
> > attach);
> 
> Not much to complain actually, happy again that we have this
> tested. Lots of pre increment/decrement. I don't care much but
> that's not that common in the client, I think.
> 

Removed (at least in this patch, tell me if you see more, not strong
on it)

> >  
> >      return g_test_run();
> >  }

Frediano