Re: [Qemu-devel] [PATCH spice-server 08/13] server: move the linking of channels to a separate routine

[Yonit Halperin <yhalperi@redhat.com>]

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
>>