[Spice-devel] [PATCH 2/3] Handle VDAgentDisplayConfig message in vdagentd, send it to active vdagent

[Dunrong Huang]

Hi,


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

> Hi, Dunrong
>
> Thanks for the review, my comments inline.
>
> On Mon, Jul 29, 2013 at 6:58 AM, Dunrong Huang <riegamaths at gmail.com>
> wrote:
> > Hi,
> >
> >
> > On Mon, Jul 29, 2013 at 4:43 AM, Fedor Lyakhov <fedor.lyakhov at gmail.com>
> > 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 at lists.freedesktop.org
> >> http://lists.freedesktop.org/mailman/listinfo/spice-devel
>


-- 
Best Regards,

Dunrong Huang

Homepage: http://mathslinux.org
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/spice-devel/attachments/20130730/8d490ef6/attachment-0001.html>