Hi, Dunrong
Thanks for the review, my comments inline.
On Mon, Jul 29, 2013 at 6:58 AM, Dunrong Huang <riegamaths@xxxxxxxxx> wrote:
> Hi,
>
>
> On Mon, Jul 29, 2013 at 4:43 AM, Fedor Lyakhov <fedor.lyakhov@xxxxxxxxx>
> 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.
OK, will add it. But won't compiler give a warning on this extra comma actually?
>>
>> };
>>
>> #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"?
Agree. Thanks for finding this. Bad copy of code from do_client_monitors().
>>
>> +
>> + /* 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
>> Spice-devel@xxxxxxxxxxxxxxxxxxxxx
>> http://lists.freedesktop.org/mailman/listinfo/spice-devel
>
>
>
>
> --
> Best Regards,
>
> Dunrong Huang
>
> Homepage: http://mathslinux.org
--
Best regards,
Fedor
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/spice-devel