Patchwork [spice-server,08/13] server: move the linking of channels to a separate routine

login
register
mail settings
Submitter Yonit Halperin
Date Sept. 21, 2011, 3:51 p.m.
Message ID <1316620283-8330-9-git-send-email-yhalperi@redhat.com>
Download mbox | patch
Permalink /patch/115820/
State New
Headers show

Comments

Yonit Halperin - Sept. 21, 2011, 3:51 p.m.
Signed-off-by: Yonit Halperin <yhalperi@redhat.com>
---
 server/reds.c |   68 +++++++++++++++++++++++++++++++++++---------------------
 1 files changed, 42 insertions(+), 26 deletions(-)
Alon Levy - Sept. 22, 2011, 2:01 p.m.
On Wed, Sep 21, 2011 at 06:51:18PM +0300, Yonit Halperin wrote:
> 
> Signed-off-by: Yonit Halperin <yhalperi@redhat.com>
> ---
>  server/reds.c |   68 +++++++++++++++++++++++++++++++++++---------------------
>  1 files changed, 42 insertions(+), 26 deletions(-)
> 
> diff --git a/server/reds.c b/server/reds.c
> index bea0eb0..e7388a0 100644
> --- a/server/reds.c
> +++ b/server/reds.c
> @@ -2612,12 +2612,48 @@ static void inputs_init()
>      reds_register_channel(channel);
>  }
>  
> +static void reds_send_input_channel_insecure_warn()
> +{
> +    RedsOutItem *item;
> +    SpiceMsgNotify notify;
> +    char *mess = "keyboard channel is insecure";
> +    const int mess_len = strlen(mess);
> +
> +    item = new_out_item(SPICE_MSG_NOTIFY);
> +
> +    notify.time_stamp = get_time_stamp();
> +    notify.severity = SPICE_NOTIFY_SEVERITY_WARN;
> +    notify.visibilty = SPICE_NOTIFY_VISIBILITY_HIGH;
> +    notify.what = SPICE_WARN_GENERAL;
> +    notify.message_len = mess_len;
> +
> +    spice_marshall_msg_notify(item->m, &notify);
> +    spice_marshaller_add(item->m, (uint8_t *)mess, mess_len + 1);
> +
> +    reds_push_pipe_item(item);
> +}
> +
> +static void reds_channel_do_link(Channel *channel, SpiceLinkMess *link_msg, RedsStream *stream)
> +{
> +    uint32_t *caps;
> +
> +    ASSERT(channel);
> +    ASSERT(link_msg);
> +    ASSERT(stream);
> +
> +    if (link_msg->channel_type == SPICE_CHANNEL_INPUTS && !stream->ssl) {
> +        reds_send_input_channel_insecure_warn();
> +    }
> +    caps = (uint32_t *)((uint8_t *)link_msg + link_msg->caps_offset);
> +    channel->link(channel, stream, FALSE, link_msg->num_common_caps,
> +                  link_msg->num_common_caps ? caps : NULL, link_msg->num_channel_caps,
> +                  link_msg->num_channel_caps ? caps + link_msg->num_common_caps : NULL);
> +}
> +
>  static void reds_handle_other_links(RedLinkInfo *link)
>  {
>      Channel *channel;
> -    RedsStream *stream;
>      SpiceLinkMess *link_mess;
> -    uint32_t *caps;
>  
>      link_mess = link->link_mess;
>  
> @@ -2635,36 +2671,16 @@ static void reds_handle_other_links(RedLinkInfo *link)
>      }
>  
>      reds_send_link_result(link, SPICE_LINK_ERR_OK);
> +    // TODO: if mig_target, where this call should be? here or only when the links are handled
Both? add a prefix argument to reds_show_new_channel maybe.

>      reds_show_new_channel(link, reds->link_id);
> -    if (link_mess->channel_type == SPICE_CHANNEL_INPUTS && !link->stream->ssl) {
> -        RedsOutItem *item;
> -        SpiceMsgNotify notify;
> -        char *mess = "keyboard channel is insecure";
> -        const int mess_len = strlen(mess);
> -
> -        item = new_out_item(SPICE_MSG_NOTIFY);
> -
> -        notify.time_stamp = get_time_stamp();
> -        notify.severity = SPICE_NOTIFY_SEVERITY_WARN;
> -        notify.visibilty = SPICE_NOTIFY_VISIBILITY_HIGH;
> -        notify.what = SPICE_WARN_GENERAL;
> -        notify.message_len = mess_len;
> +    reds_stream_remove_watch(link->stream);
>  
> -        spice_marshall_msg_notify(item->m, &notify);
> -        spice_marshaller_add(item->m, (uint8_t *)mess, mess_len + 1);
> +    reds_channel_do_link(channel, link->link_mess, link->stream);

Would be nice to consistently use link_mess or link->link_mess, not both.

> +    free(link_mess);
>  
> -        reds_push_pipe_item(item);
> -    }
> -    stream = link->stream;
> -    reds_stream_remove_watch(stream);
>      link->stream = NULL;
>      link->link_mess = NULL;
>      reds_link_free(link);
> -    caps = (uint32_t *)((uint8_t *)link_mess + link_mess->caps_offset);
> -    channel->link(channel, stream, reds->mig_target, link_mess->num_common_caps,
> -                  link_mess->num_common_caps ? caps : NULL, link_mess->num_channel_caps,
> -                  link_mess->num_channel_caps ? caps + link_mess->num_common_caps : NULL);
> -    free(link_mess);
>  }
>  
>  static void reds_handle_link(RedLinkInfo *link)
> -- 
> 1.7.4.4
>
Yonit Halperin - Sept. 25, 2011, 9:13 a.m.
On 09/22/2011 05:01 PM, Alon Levy wrote:
> On Wed, Sep 21, 2011 at 06:51:18PM +0300, Yonit Halperin wrote:
>>
>> Signed-off-by: Yonit Halperin<yhalperi@redhat.com>
>> ---
>>   server/reds.c |   68 +++++++++++++++++++++++++++++++++++---------------------
>>   1 files changed, 42 insertions(+), 26 deletions(-)
>>
>> diff --git a/server/reds.c b/server/reds.c
>> index bea0eb0..e7388a0 100644
>> --- a/server/reds.c
>> +++ b/server/reds.c
>> @@ -2612,12 +2612,48 @@ static void inputs_init()
>>       reds_register_channel(channel);
>>   }
>>
>> +static void reds_send_input_channel_insecure_warn()
>> +{
>> +    RedsOutItem *item;
>> +    SpiceMsgNotify notify;
>> +    char *mess = "keyboard channel is insecure";
>> +    const int mess_len = strlen(mess);
>> +
>> +    item = new_out_item(SPICE_MSG_NOTIFY);
>> +
>> +    notify.time_stamp = get_time_stamp();
>> +    notify.severity = SPICE_NOTIFY_SEVERITY_WARN;
>> +    notify.visibilty = SPICE_NOTIFY_VISIBILITY_HIGH;
>> +    notify.what = SPICE_WARN_GENERAL;
>> +    notify.message_len = mess_len;
>> +
>> +    spice_marshall_msg_notify(item->m,&notify);
>> +    spice_marshaller_add(item->m, (uint8_t *)mess, mess_len + 1);
>> +
>> +    reds_push_pipe_item(item);
>> +}
>> +
>> +static void reds_channel_do_link(Channel *channel, SpiceLinkMess *link_msg, RedsStream *stream)
>> +{
>> +    uint32_t *caps;
>> +
>> +    ASSERT(channel);
>> +    ASSERT(link_msg);
>> +    ASSERT(stream);
>> +
>> +    if (link_msg->channel_type == SPICE_CHANNEL_INPUTS&&  !stream->ssl) {
>> +        reds_send_input_channel_insecure_warn();
>> +    }
>> +    caps = (uint32_t *)((uint8_t *)link_msg + link_msg->caps_offset);
>> +    channel->link(channel, stream, FALSE, link_msg->num_common_caps,
>> +                  link_msg->num_common_caps ? caps : NULL, link_msg->num_channel_caps,
>> +                  link_msg->num_channel_caps ? caps + link_msg->num_common_caps : NULL);
>> +}
>> +
>>   static void reds_handle_other_links(RedLinkInfo *link)
>>   {
>>       Channel *channel;
>> -    RedsStream *stream;
>>       SpiceLinkMess *link_mess;
>> -    uint32_t *caps;
>>
>>       link_mess = link->link_mess;
>>
>> @@ -2635,36 +2671,16 @@ static void reds_handle_other_links(RedLinkInfo *link)
>>       }
>>
>>       reds_send_link_result(link, SPICE_LINK_ERR_OK);
>> +    // TODO: if mig_target, where this call should be? here or only when the links are handled
> Both? add a prefix argument to reds_show_new_channel maybe.
>
Hi, my concern was more about the red_channel_event called from 
reds_show_new_channel. I think it should be called only once, and at the 
same place it is in these patches - when the client makes the initial 
connection to the target, i.e., before migration.
>>       reds_show_new_channel(link, reds->link_id);
>> -    if (link_mess->channel_type == SPICE_CHANNEL_INPUTS&&  !link->stream->ssl) {
>> -        RedsOutItem *item;
>> -        SpiceMsgNotify notify;
>> -        char *mess = "keyboard channel is insecure";
>> -        const int mess_len = strlen(mess);
>> -
>> -        item = new_out_item(SPICE_MSG_NOTIFY);
>> -
>> -        notify.time_stamp = get_time_stamp();
>> -        notify.severity = SPICE_NOTIFY_SEVERITY_WARN;
>> -        notify.visibilty = SPICE_NOTIFY_VISIBILITY_HIGH;
>> -        notify.what = SPICE_WARN_GENERAL;
>> -        notify.message_len = mess_len;
>> +    reds_stream_remove_watch(link->stream);
>>
>> -        spice_marshall_msg_notify(item->m,&notify);
>> -        spice_marshaller_add(item->m, (uint8_t *)mess, mess_len + 1);
>> +    reds_channel_do_link(channel, link->link_mess, link->stream);
>
> Would be nice to consistently use link_mess or link->link_mess, not both.
>
>> +    free(link_mess);
>>
>> -        reds_push_pipe_item(item);
>> -    }
>> -    stream = link->stream;
>> -    reds_stream_remove_watch(stream);
>>       link->stream = NULL;
>>       link->link_mess = NULL;
>>       reds_link_free(link);
>> -    caps = (uint32_t *)((uint8_t *)link_mess + link_mess->caps_offset);
>> -    channel->link(channel, stream, reds->mig_target, link_mess->num_common_caps,
>> -                  link_mess->num_common_caps ? caps : NULL, link_mess->num_channel_caps,
>> -                  link_mess->num_channel_caps ? caps + link_mess->num_common_caps : NULL);
>> -    free(link_mess);
>>   }
>>
>>   static void reds_handle_link(RedLinkInfo *link)
>> --
>> 1.7.4.4
>>

Patch

diff --git a/server/reds.c b/server/reds.c
index bea0eb0..e7388a0 100644
--- a/server/reds.c
+++ b/server/reds.c
@@ -2612,12 +2612,48 @@  static void inputs_init()
     reds_register_channel(channel);
 }
 
+static void reds_send_input_channel_insecure_warn()
+{
+    RedsOutItem *item;
+    SpiceMsgNotify notify;
+    char *mess = "keyboard channel is insecure";
+    const int mess_len = strlen(mess);
+
+    item = new_out_item(SPICE_MSG_NOTIFY);
+
+    notify.time_stamp = get_time_stamp();
+    notify.severity = SPICE_NOTIFY_SEVERITY_WARN;
+    notify.visibilty = SPICE_NOTIFY_VISIBILITY_HIGH;
+    notify.what = SPICE_WARN_GENERAL;
+    notify.message_len = mess_len;
+
+    spice_marshall_msg_notify(item->m, &notify);
+    spice_marshaller_add(item->m, (uint8_t *)mess, mess_len + 1);
+
+    reds_push_pipe_item(item);
+}
+
+static void reds_channel_do_link(Channel *channel, SpiceLinkMess *link_msg, RedsStream *stream)
+{
+    uint32_t *caps;
+
+    ASSERT(channel);
+    ASSERT(link_msg);
+    ASSERT(stream);
+
+    if (link_msg->channel_type == SPICE_CHANNEL_INPUTS && !stream->ssl) {
+        reds_send_input_channel_insecure_warn();
+    }
+    caps = (uint32_t *)((uint8_t *)link_msg + link_msg->caps_offset);
+    channel->link(channel, stream, FALSE, link_msg->num_common_caps,
+                  link_msg->num_common_caps ? caps : NULL, link_msg->num_channel_caps,
+                  link_msg->num_channel_caps ? caps + link_msg->num_common_caps : NULL);
+}
+
 static void reds_handle_other_links(RedLinkInfo *link)
 {
     Channel *channel;
-    RedsStream *stream;
     SpiceLinkMess *link_mess;
-    uint32_t *caps;
 
     link_mess = link->link_mess;
 
@@ -2635,36 +2671,16 @@  static void reds_handle_other_links(RedLinkInfo *link)
     }
 
     reds_send_link_result(link, SPICE_LINK_ERR_OK);
+    // TODO: if mig_target, where this call should be? here or only when the links are handled
     reds_show_new_channel(link, reds->link_id);
-    if (link_mess->channel_type == SPICE_CHANNEL_INPUTS && !link->stream->ssl) {
-        RedsOutItem *item;
-        SpiceMsgNotify notify;
-        char *mess = "keyboard channel is insecure";
-        const int mess_len = strlen(mess);
-
-        item = new_out_item(SPICE_MSG_NOTIFY);
-
-        notify.time_stamp = get_time_stamp();
-        notify.severity = SPICE_NOTIFY_SEVERITY_WARN;
-        notify.visibilty = SPICE_NOTIFY_VISIBILITY_HIGH;
-        notify.what = SPICE_WARN_GENERAL;
-        notify.message_len = mess_len;
+    reds_stream_remove_watch(link->stream);
 
-        spice_marshall_msg_notify(item->m, &notify);
-        spice_marshaller_add(item->m, (uint8_t *)mess, mess_len + 1);
+    reds_channel_do_link(channel, link->link_mess, link->stream);
+    free(link_mess);
 
-        reds_push_pipe_item(item);
-    }
-    stream = link->stream;
-    reds_stream_remove_watch(stream);
     link->stream = NULL;
     link->link_mess = NULL;
     reds_link_free(link);
-    caps = (uint32_t *)((uint8_t *)link_mess + link_mess->caps_offset);
-    channel->link(channel, stream, reds->mig_target, link_mess->num_common_caps,
-                  link_mess->num_common_caps ? caps : NULL, link_mess->num_channel_caps,
-                  link_mess->num_channel_caps ? caps + link_mess->num_common_caps : NULL);
-    free(link_mess);
 }
 
 static void reds_handle_link(RedLinkInfo *link)