Re: [PATCH spice-gtk v2 3/4] uri: generate spice://host:port or spice+tls://host:port

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

 



On Wed, Feb 21, 2018 at 10:17 AM, Frediano Ziglio <fziglio@xxxxxxxxxx> wrote:
>> From: Marc-André Lureau <marcandre.lureau@xxxxxxxxxx>
>>
>> Signed-off-by: Marc-André Lureau <marcandre.lureau@xxxxxxxxxx>
>> ---
>>  src/spice-session.c | 31 +++++++++++++++++++++++--------
>>  tests/session.c     | 20 ++++++++++----------
>>  2 files changed, 33 insertions(+), 18 deletions(-)
>>
>> diff --git a/src/spice-session.c b/src/spice-session.c
>> index d2aa5e7..6faf0f2 100644
>> --- a/src/spice-session.c
>> +++ b/src/spice-session.c
>> @@ -400,17 +400,32 @@ static gchar* spice_uri_create(SpiceSession *session)
>>      if (s->unix_path != NULL) {
>>          return g_strdup_printf(URI_SCHEME_SPICE_UNIX "%s", s->unix_path);
>>      } else if (s->host != NULL) {
>> +        GString *str;
>>          g_return_val_if_fail(s->port != NULL || s->tls_port != NULL, NULL);
>>
>> -        GString *str = g_string_new(URI_SCHEME_SPICE);
>> +        if (!!s->tls_port + !!s->port == 1) {
>> +            /* use spice://foo:4390 or spice+tls://.. form */
>> +            const char *port;
>>
>> -        g_string_append(str, s->host);
>> -        g_string_append(str, "?");
>> -        if (s->port != NULL) {
>> -            g_string_append_printf(str, "port=%s&", s->port);
>> -        }
>> -        if (s->tls_port != NULL) {
>> -            g_string_append_printf(str, "tls-port=%s", s->tls_port);
>> +            if (s->tls_port) {
>> +                str = g_string_new(URI_SCHEME_SPICE_TLS);
>> +                port = s->tls_port;
>> +            } else {
>> +                str = g_string_new(URI_SCHEME_SPICE);
>> +                port = s->port;
>> +            }
>> +            g_string_append_printf(str, "%s:%s", s->host, port);
>> +        } else {
>> +            /* use spice://foo?port=4390&tls-port= form */
>> +            str = g_string_new(URI_SCHEME_SPICE);
>> +            g_string_append(str, s->host);
>> +            g_string_append(str, "?");
>> +            if (s->port != NULL) {
>> +                g_string_append_printf(str, "port=%s&", s->port);
>> +            }
>> +            if (s->tls_port != NULL) {
>> +                g_string_append_printf(str, "tls-port=%s", s->tls_port);
>> +            }
>>          }
>>          return g_string_free(str, FALSE);
>>      }
>
> Looks more complicated than should be, specially the !!s->tls_port + !!s->port == 1
> check. Considered that the row above remove the case where none are set
> the check can just be if both are set and the reverse, I propose (tested) a
>
>

Indeed, thanks for the suggestion!

>     if (s->unix_path != NULL) {
>         return g_strdup_printf(URI_SCHEME_SPICE_UNIX "%s", s->unix_path);
>     } else if (s->host != NULL) {
>         const char *port, *scheme;
>         g_return_val_if_fail(s->port != NULL || s->tls_port != NULL, NULL);
>
>         if (s->tls_port && s->port) {
>             /* both set, use spice://foo?port=4390&tls-port= form */
>             return g_strdup_printf(URI_SCHEME_SPICE "%s?port=%s&tls-port=%s",
>                                    s->host, s->port, s->tls_port);
>         }
>
>         /* one set, use spice://foo:4390 or spice+tls://.. form */
>         if (s->tls_port) {
>             scheme = URI_SCHEME_SPICE_TLS;
>             port = s->tls_port;
>         } else {
>             scheme = URI_SCHEME_SPICE;
>             port = s->port;
>         }
>         return g_strdup_printf("%s%s:%s", scheme, s->host, port);
>     }
>
>> diff --git a/tests/session.c b/tests/session.c
>> index 413d812..fc874fc 100644
>> --- a/tests/session.c
>> +++ b/tests/session.c
>> @@ -201,28 +201,28 @@ static void test_session_uri_ipv4_good(void)
>>            "localhost",
>>            NULL, NULL,
>>            "spice://localhost?port=5900&tls-port=",
>> -          "spice://localhost?port=5900&" },
>> +          "spice://localhost:5900" },
>>          { "5910", NULL,
>>            "localhost",
>>            "user", NULL,
>>            "spice://user@localhost?tls-port=&port=5910",
>> -          "spice://localhost?port=5910&" },
>> +          "spice://localhost:5910" },
>>          { NULL, "5920",
>>            "localhost",
>>            "user", "password",
>>            "spice://user@localhost?tls-port=5920&port=&password=password",
>> -          "spice://localhost?tls-port=5920",
>> +          "spice+tls://localhost:5920",
>>            "password may be visible in process listings"},
>>          { NULL, "5930",
>>            "localhost",
>>            NULL, NULL,
>>            "spice://localhost?port=&tls-port=5930",
>> -          "spice://localhost?tls-port=5930" },
>> +          "spice+tls://localhost:5930" },
>>          { "42", NULL,
>>            "localhost",
>>            NULL, NULL,
>>            "spice://localhost:42",
>> -          "spice://localhost?port=42&" },
>> +          "spice://localhost:42" },
>>          { "42", "5930",
>>            "localhost",
>>            NULL, NULL,
>> @@ -246,28 +246,28 @@ static void test_session_uri_ipv6_good(void)
>>            "[2010:836B:4179::836B:4179]",
>>            NULL, NULL,
>>            "spice://[2010:836B:4179::836B:4179]?port=5900&tls-port=",
>> -          "spice://[2010:836B:4179::836B:4179]?port=5900&" },
>> +          "spice://[2010:836B:4179::836B:4179]:5900" },
>>          { "5910", NULL,
>>            "[::192.9.5.5]",
>>            "user", NULL,
>>            "spice://user@[::192.9.5.5]?tls-port=&port=5910",
>> -          "spice://[::192.9.5.5]?port=5910&" },
>> +          "spice://[::192.9.5.5]:5910" },
>>          { NULL, "5920",
>>            "[3ffe:2a00:100:7031::1]",
>>            "user", "password",
>>            "spice://user@[3ffe:2a00:100:7031::1]?tls-port=5920&port=&password=password",
>> -          "spice://[3ffe:2a00:100:7031::1]?tls-port=5920",
>> +          "spice+tls://[3ffe:2a00:100:7031::1]:5920",
>>            "password may be visible in process listings"},
>>          { NULL, "5930",
>>            "[1080:0:0:0:8:800:200C:417A]",
>>            NULL, NULL,
>>            "spice://[1080:0:0:0:8:800:200C:417A]?port=&tls-port=5930",
>> -          "spice://[1080:0:0:0:8:800:200C:417A]?tls-port=5930" },
>> +          "spice+tls://[1080:0:0:0:8:800:200C:417A]:5930" },
>>          { "42", NULL,
>>            "[FEDC:BA98:7654:3210:FEDC:BA98:7654:3210]",
>>            NULL, NULL,
>>            "spice://[FEDC:BA98:7654:3210:FEDC:BA98:7654:3210]:42",
>> -          "spice://[FEDC:BA98:7654:3210:FEDC:BA98:7654:3210]?port=42&" },
>> +          "spice://[FEDC:BA98:7654:3210:FEDC:BA98:7654:3210]:42" },
>>          { "42", "5930",
>>            "[::192.9.5.5]",
>>            NULL, NULL,
>
> Frediano
> _______________________________________________
> Spice-devel mailing list
> Spice-devel@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/spice-devel



-- 
Marc-André Lureau
_______________________________________________
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]