<div dir="ltr">Hi, <div class="gmail_extra"> <div class="gmail_quote">On Tue, Jul 30, 2013 at 5:00 AM, Fedor Lyakhov <<a href="mailto:fedor.lyakhov@gmail.com" target="_blank">fedor.lyakhov@gmail.com</a>> wrote: <blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">Hi, Dunrong Thanks for the review, my comments inline. <div class="im"> On Mon, Jul 29, 2013 at 6:58 AM, Dunrong Huang <<a href="mailto:riegamaths@gmail.com">riegamaths@gmail.com</a>> wrote: > Hi, > > > On Mon, Jul 29, 2013 at 4:43 AM, Fedor Lyakhov <<a href="mailto:fedor.lyakhov@gmail.com">fedor.lyakhov@gmail.com</a>> > wrote: >> >> --- >> src/vdagentd-proto-strings.h | 1 + >> src/vdagentd-proto.h | 3 ++- >> src/vdagentd.c | 38 ++++++++++++++++++++++++++++++++++++++ >> 3 files changed, 41 insertions(+), 1 deletion(-) >> >> diff --git a/src/vdagentd-proto-strings.h b/src/vdagentd-proto-strings.h >> index e76cb3b..7ea7195 100644 >> --- a/src/vdagentd-proto-strings.h >> +++ b/src/vdagentd-proto-strings.h >> @@ -34,6 +34,7 @@ static const char * const vdagentd_messages[] = { >> "file xfer status", >> "file xfer data", >> "client disconnected", >> + "display config" > > Adding a comma at the end will be better, so next time someone don't need to > change this line when they add something. </div>OK, will add it. But won't compiler give a warning on this extra comma actually? </blockquote><div>It's common way for array initialization, no harm to compiler.</div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"> <div><div class="h5"> >> >> }; >> >> #endif >> diff --git a/src/vdagentd-proto.h b/src/vdagentd-proto.h >> index 25e6a36..6a09e49 100644 >> --- a/src/vdagentd-proto.h >> +++ b/src/vdagentd-proto.h >> @@ -39,7 +39,8 @@ enum { >> VDAGENTD_FILE_XFER_START, >> VDAGENTD_FILE_XFER_STATUS, >> VDAGENTD_FILE_XFER_DATA, >> - VDAGENTD_CLIENT_DISCONNECTED, /* daemon -> client */ >> + VDAGENTD_CLIENT_DISCONNECTED, /* daemon -> client */ >> + VDAGENTD_DISPLAY_CONFIG, /* daemon -> client, VDAgentDisplayConfig */ >> VDAGENTD_NO_MESSAGES /* Must always be last */ >> }; >> >> diff --git a/src/vdagentd.c b/src/vdagentd.c >> index f4cea44..c9df401 100644 >> --- a/src/vdagentd.c >> +++ b/src/vdagentd.c >> @@ -91,6 +91,7 @@ static void send_capabilities(struct >> vdagent_virtio_port *vport, >> caps->request = request; >> VD_AGENT_SET_CAPABILITY(caps->caps, VD_AGENT_CAP_MOUSE_STATE); >> VD_AGENT_SET_CAPABILITY(caps->caps, VD_AGENT_CAP_MONITORS_CONFIG); >> + VD_AGENT_SET_CAPABILITY(caps->caps, VD_AGENT_CAP_DISPLAY_CONFIG); >> VD_AGENT_SET_CAPABILITY(caps->caps, VD_AGENT_CAP_REPLY); >> VD_AGENT_SET_CAPABILITY(caps->caps, >> VD_AGENT_CAP_CLIPBOARD_BY_DEMAND); >> VD_AGENT_SET_CAPABILITY(caps->caps, >> VD_AGENT_CAP_CLIPBOARD_SELECTION); >> @@ -151,6 +152,38 @@ static void do_client_monitors(struct >> vdagent_virtio_port *vport, int port_nr, >> (uint8_t *)&reply, sizeof(reply)); >> } >> >> +static void do_client_display(struct vdagent_virtio_port *vport, int >> port_nr, >> + VDAgentMessage *message_header, VDAgentDisplayConfig *disp) >> +{ >> + VDAgentReply reply; >> + VDAgentDisplayConfig *display_config; >> + uint32_t size; >> + >> + size = sizeof(VDAgentDisplayConfig); >> + if (message_header->size != size) { >> + syslog(LOG_ERR, "invalid message size for VDAgentDisplayConfig"); >> + return; >> + } >> + >> + display_config = malloc(size); >> >> + if (!display_config) { >> + syslog(LOG_ERR, "oom allocating display config"); >> + size = 0; >> + } >> + memcpy(display_config, disp, size); >> + >> + /* Send display config to currently active agent */ >> + if (active_session_conn) >> + udscs_write(active_session_conn, VDAGENTD_DISPLAY_CONFIG, 0, 0, >> + (uint8_t *)display_config, size); > > display_config will be leaked, why not just use the "disp"? </div></div>Agree. Thanks for finding this. Bad copy of code from do_client_monitors(). <div class=""><div class="h5"> >> >> + >> + /* Acknowledge reception of display config to spice server / client >> */ >> + reply.type = VD_AGENT_DISPLAY_CONFIG; >> + reply.error = VD_AGENT_SUCCESS; >> + vdagent_virtio_port_write(vport, port_nr, VD_AGENT_REPLY, 0, >> + (uint8_t *)&reply, sizeof(reply)); >> +} >> + >> static void do_client_capabilities(struct vdagent_virtio_port *vport, >> VDAgentMessage *message_header, >> VDAgentAnnounceCapabilities *caps) >> @@ -330,6 +363,11 @@ int virtio_port_read_complete( >> do_client_monitors(vport, port_nr, message_header, >> (VDAgentMonitorsConfig *)data); >> break; >> + case VD_AGENT_DISPLAY_CONFIG: >> + if (message_header->size < sizeof(VDAgentDisplayConfig)) >> + goto size_error; >> + do_client_display(vport, port_nr, message_header, >> + (VDAgentDisplayConfig *)data); >> case VD_AGENT_ANNOUNCE_CAPABILITIES: >> if (message_header->size < sizeof(VDAgentAnnounceCapabilities)) >> goto size_error; >> -- >> 1.8.1.4 >> _______________________________________________ >> Spice-devel mailing list >> <a href="mailto:Spice-devel@lists.freedesktop.org">Spice-devel@lists.freedesktop.org</a> >> <a href="http://lists.freedesktop.org/mailman/listinfo/spice-devel" target="_blank">http://lists.freedesktop.org/mailman/listinfo/spice-devel</a> </div></div></blockquote></div> <div> </div> -- <div dir="ltr">Best Regards,<div> Dunrong Huang <div> <div>Homepage: <a href="http://mathslinux.org" target="_blank">http://mathslinux.org</a></div></div></div></div> </div></div>