[Spice-devel] [PATCH spice-gtk 1/4] tests: add spice+unix:// URI checks

[Frediano Ziglio]

> 
> Hi
> 
> On Thu, Feb 8, 2018 at 2:18 PM, Frediano Ziglio <fziglio at redhat.com> wrote:
> >>
> >> From: Marc-André Lureau <marcandre.lureau at redhat.com>
> >>
> >> For some reason, the URIs test didn't include spice+unix:// checks,
> >> probably because they came about the same time.
> >>
> >> Signed-off-by: Marc-André Lureau <marcandre.lureau at redhat.com>
> >> ---
> >>  tests/session.c | 28 +++++++++++++++++++++++++---
> >>  1 file changed, 25 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/tests/session.c b/tests/session.c
> >> index 7ed4a41..413d812 100644
> >> --- a/tests/session.c
> >> +++ b/tests/session.c
> >> @@ -9,6 +9,7 @@ typedef struct {
> >>      const gchar *uri_input;
> >>      const gchar *uri_output;
> >>      const gchar *message;
> >> +    const gchar *unix_path;
> >>  } TestCase;
> >>
> >>  static void test_session_uri_bad(void)
> >> @@ -139,7 +140,7 @@ static void test_session_uri_good(const TestCase
> >> *tests,
> >> const guint cases)
> >>
> >>      /* Set URI and check URI, port and tls_port */
> >>      for (i = 0; i < cases; i++) {
> >> -        gchar *uri, *port, *tls_port, *host, *username, *password;
> >> +        gchar *uri, *port, *tls_port, *host, *username, *password,
> >> *unix_path;
> >>
> >>          s = spice_session_new();
> >>          if (tests[i].message != NULL)
> >> @@ -152,20 +153,23 @@ static void test_session_uri_good(const TestCase
> >> *tests, const guint cases)
> >>                       "host", &host,
> >>                       "username", &username,
> >>                       "password", &password,
> >> +                     "unix-path", &unix_path,
> >>                        NULL);
> >> -        g_assert_cmpstr(tests[i].uri_output, ==, uri);
> >> +        g_assert_cmpstr(tests[i].uri_output ?: tests[i].uri_input, ==,
> >> uri);
> >>          g_assert_cmpstr(tests[i].port, ==, port);
> >>          g_assert_cmpstr(tests[i].tls_port, ==, tls_port);
> >>          g_assert_cmpstr(tests[i].host, ==, host);
> >>          g_assert_cmpstr(tests[i].username, ==, username);
> >>          g_assert_cmpstr(tests[i].password, ==, password);
> >>          g_test_assert_expected_messages();
> >> +        g_assert_cmpstr(tests[i].unix_path, ==, unix_path);
> >>          g_clear_pointer(&uri, g_free);
> >>          g_clear_pointer(&port, g_free);
> >>          g_clear_pointer(&tls_port, g_free);
> >>          g_clear_pointer(&host, g_free);
> >>          g_clear_pointer(&username, g_free);
> >>          g_clear_pointer(&password, g_free);
> >> +        g_clear_pointer(&unix_path, g_free);
> >>          g_object_unref(s);
> >>      }
> >>
> >> @@ -180,9 +184,10 @@ static void test_session_uri_good(const TestCase
> >> *tests,
> >> const guint cases)
> >>                       "host", tests[i].host,
> >>                       "username", tests[i].username,
> >>                       "password", tests[i].password,
> >> +                     "unix-path", tests[i].unix_path,
> >>                        NULL);
> >>          g_object_get(s, "uri", &uri, NULL);
> >> -        g_assert_cmpstr(tests[i].uri_output, ==, uri);
> >> +        g_assert_cmpstr(tests[i].uri_output ?: tests[i].uri_input, ==,
> >> uri);
> >>          g_clear_pointer(&uri, g_free);
> >>          g_object_unref(s);
> >>      }
> >> @@ -278,6 +283,22 @@ static void test_session_uri_ipv6_good(void)
> >>      test_session_uri_good(tests, G_N_ELEMENTS(tests));
> >>  }
> >>
> >> +static void test_session_uri_unix_good(void)
> >> +{
> >> +    const TestCase tests[] = {
> >> +        { .uri_input = "spice+unix:///tmp/foo.sock",
> >> +          .unix_path = "/tmp/foo.sock" },
> >> +        /* perhaps not very clever, but this doesn't raise an
> >> error/warning
> >> */
> >> +        { .uri_input = "spice+unix://",
> >> +          .unix_path = "" },
> >> +        /* unix uri don't support passing password or other kind of
> >> options
> >> */
> >> +        { .uri_input = "spice+unix:///tmp/foo.sock?password=frobnicate",
> >> +          .unix_path = "/tmp/foo.sock?password=frobnicate" },
> >> +    };
> >> +
> >> +    test_session_uri_good(tests, G_N_ELEMENTS(tests));
> >> +}
> >> +
> >>  int main(int argc, char* argv[])
> >>  {
> >>      g_test_init(&argc, &argv, NULL);
> >> @@ -285,6 +306,7 @@ int main(int argc, char* argv[])
> >>      g_test_add_func("/session/bad-uri", test_session_uri_bad);
> >>      g_test_add_func("/session/good-ipv4-uri",
> >>      test_session_uri_ipv4_good);
> >>      g_test_add_func("/session/good-ipv6-uri",
> >>      test_session_uri_ipv6_good);
> >> +    g_test_add_func("/session/good-unix", test_session_uri_unix_good);
> >>
> >>      return g_test_run();
> >>  }
> >
> > Looks good (still to review better).
> > Can we consider this patch separate from the rest of the series
> > (that is merge even separately) ?
> 
> Sure, it was just in the same area of code, and thus added dependency,
> but we can review & merge this one right away I think.
> thanks
> 

Acked-by: Frediano Ziglio <fziglio at redhat.com>

Follow ups:

The "spice://" syntax is weird, maybe we should refuse it.
Running remote-viewer with this URI attempts to open an abstract
socket with no name which result in a failure in any case.

The not support for query strings could be a problem for the future,
I don't think not accepting the "?" inside a path would lead to
issues, maybe we could accept the query string syntax to be able
to pass options in the future. If you think Xorg uses unix sockets
too but on top of it does authentication of the clients.
We could accept %3f syntax (0x3f is ascii for '?') if we really
want to accept the question mark.

Frediano