From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:41116) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1R7kmN-00076A-5e for qemu-devel@nongnu.org; Sun, 25 Sep 2011 05:13:44 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1R7kmL-0007g6-UV for qemu-devel@nongnu.org; Sun, 25 Sep 2011 05:13:43 -0400 Received: from mx1.redhat.com ([209.132.183.28]:36904) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1R7kmL-0007fc-Ip for qemu-devel@nongnu.org; Sun, 25 Sep 2011 05:13:41 -0400 Message-ID: <4E7EF0CE.1010400@redhat.com> Date: Sun, 25 Sep 2011 12:13:50 +0300 From: Yonit Halperin MIME-Version: 1.0 References: <1316620283-8330-1-git-send-email-yhalperi@redhat.com> <1316620283-8330-9-git-send-email-yhalperi@redhat.com> <20110922140100.GD2875@bow.tlv.redhat.com> In-Reply-To: <20110922140100.GD2875@bow.tlv.redhat.com> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH spice-server 08/13] server: move the linking of channels to a separate routine List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: spice-devel@freedesktop.org, qemu-devel@nongnu.org, kraxel@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 >> --- >> 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,¬ify); >> + 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,¬ify); >> - 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 >>