Re: [PATCH 2/3] Handle VDAgentDisplayConfig message in vdagentd, send it to active vdagent

[Dunrong Huang <riegamaths@xxxxxxxxx>]

Hi,

On Tue, Jul 30, 2013 at 5:00 AM, Fedor Lyakhov <fedor.lyakhov@xxxxxxxxx> wrote:

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?

It's common way for array initialization, no harm to compiler.

>>
>>  };
>>
>>  #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

_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/spice-devel