[Spice-devel] [PATCH spice-gtk 1/4] tests: add spice+unix:// URI checks
Frediano Ziglio
fziglio at redhat.com
Wed Feb 14 07:49:04 UTC 2018
>
> Hi
>
> On Tue, Feb 13, 2018 at 10:08 AM, Frediano Ziglio <fziglio at redhat.com> wrote:
> >>
> >> 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.
>
> 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
>
Looks like you nack (for the moment) your series.
Personally between vnc URI syntax and rdp one I prefer vnc one, looks
more "standard".
If we are going in this direction I would accept query strings
in any case (even with unix sockets).
Frediano
More information about the Spice-devel
mailing list