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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi

On Tue, Feb 13, 2018 at 10:08 AM, Frediano Ziglio <fziglio@xxxxxxxxxx> wrote:
>>
>> Hi
>>
>> On Thu, Feb 8, 2018 at 2:18 PM, Frediano Ziglio <fziglio@xxxxxxxxxx> wrote:
>> >>
>> >> From: Marc-André Lureau <marcandre.lureau@xxxxxxxxxx>
>> >>
>> >> 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@xxxxxxxxxx>
>> >> ---
>> >>  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@xxxxxxxxxx>
>
> 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.

I was not really fond of passing options via URI, but apparently this
is a standard practice. And I can imagine some of the merits. Things
have evolved quite a bit regarding URIs, Iordan (aSpice developer
among other things) even wrote a RFC for vnc!
https://tools.ietf.org/html/rfc7869. And there is also a microsoft
document for RDP:
https://docs.microsoft.com/en-us/windows-server/remote/remote-desktop-services/clients/remote-desktop-uri.

So we could follow that trend, and we could/should accept more options
as URI. At least, I'll revise the rest of the spice+tls series. (btw,
it looks like vnc uses a SecurityType=enum argument instead of vnc+tls
scheme for example - not sure which one is preferable, but I like the
+ better)

Ioardan, do you have any opinion on "spice" URI ?

thanks
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel




[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux]     [Linux OMAP]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]     [Monitors]