Hi,
On Thu, 2019-01-10 at 15:27 -0600, Jonathon Jongsma wrote:
> On Tue, 2019-01-08 at 16:28 +0100, Lukáš Hrázký wrote:
> > Receives the GraphicsDeviceInfo message from the streaming agent and
> > stores the data in a list on the streaming device.
> >
> > Signed-off-by: Lukáš Hrázký <lhrazky@xxxxxxxxxx>
> > ---
> > server/red-qxl.c | 1 -
> > server/red-stream-device.c | 66
> > ++++++++++++++++++++++++++++++++++++--
> > server/red-stream-device.h | 8 +++++
> > server/reds.h | 3 ++
> > 4 files changed, 74 insertions(+), 4 deletions(-)
> >
> > diff --git a/server/red-qxl.c b/server/red-qxl.c
> > index ebc14a46..e174758f 100644
> > --- a/server/red-qxl.c
> > +++ b/server/red-qxl.c
> > @@ -41,7 +41,6 @@
> > #include "red-qxl.h"
> >
> >
> > -#define MAX_DEVICE_ADDRESS_LEN 256
> > #define MAX_MONITORS_COUNT 16
> >
> > struct QXLState {
> > diff --git a/server/red-stream-device.c b/server/red-stream-device.c
> > index 215ddbe7..bbd6874f 100644
> > --- a/server/red-stream-device.c
> > +++ b/server/red-stream-device.c
> > @@ -23,7 +23,6 @@
> >
> > #include "stream-channel.h"
> > #include "cursor-channel.h"
> > -#include "reds.h"
>
> As far as I can see, we're only adding new code in this file while
> removing an include. That tells me that this include change is
> unrelated to the other changes?
That include was actually moved to the header, see also the discussion
with Frediano if interested, I'll be changing it and this won't be here
anymore.
> > #define MAX_GUEST_CAPABILITIES_BYTES ((STREAM_CAP_END+7)/8)
> >
> > @@ -37,6 +36,7 @@ struct StreamDevice {
> > StreamMsgCapabilities capabilities;
> > StreamMsgCursorSet cursor_set;
> > StreamMsgCursorMove cursor_move;
> > + StreamMsgDeviceDisplayInfo device_display_info;
> > uint8_t buf[STREAM_MSG_CAPABILITIES_MAX_BYTES];
> > } *msg;
> > uint32_t msg_pos;
> > @@ -49,6 +49,7 @@ struct StreamDevice {
> > CursorChannel *cursor_channel;
> > SpiceTimer *close_timer;
> > uint32_t frame_mmtime;
> > + StreamDeviceDisplayInfo device_display_info;
>
> It strikes me as slightly confusing to have both
> StreamDevice.device_display_info and
> StreamDevice.msg.device_display_info
Well, one is in the message as it's being read from the virtio port,
the other is the struct that holds the data in memory. They contain the
same thing. I don't think it's confusing given you understand the
memory structure and how the messages are read and actually kind of
makes sense... It does tell you it's the same thing, just demarshalled
into memory.
Any suggestions to improve it?
Cheers,
Lukas
> > };
> >
> > struct StreamDeviceClass {
> > @@ -64,8 +65,8 @@ static void char_device_set_state(RedCharDevice
> > *char_dev, int state);
> > typedef bool StreamMsgHandler(StreamDevice *dev,
> > SpiceCharDeviceInstance *sin)
> > SPICE_GNUC_WARN_UNUSED_RESULT;
> >
> > -static StreamMsgHandler handle_msg_format, handle_msg_data,
> > handle_msg_cursor_set,
> > - handle_msg_cursor_move, handle_msg_capabilities;
> > +static StreamMsgHandler handle_msg_format,
> > handle_msg_device_display_info, handle_msg_data,
> > + handle_msg_cursor_set, handle_msg_cursor_move,
> > handle_msg_capabilities;
> >
> > static bool handle_msg_invalid(StreamDevice *dev,
> > SpiceCharDeviceInstance *sin,
> > const char *error_msg)
> > SPICE_GNUC_WARN_UNUSED_RESULT;
> > @@ -146,6 +147,13 @@ stream_device_partial_read(StreamDevice *dev,
> > SpiceCharDeviceInstance *sin)
> > handled = handle_msg_format(dev, sin);
> > }
> > break;
> > + case STREAM_TYPE_DEVICE_DISPLAY_INFO:
> > + if (dev->hdr.size > sizeof(StreamMsgDeviceDisplayInfo) +
> > MAX_DEVICE_ADDRESS_LEN) {
> > + handled = handle_msg_invalid(dev, sin,
> > "StreamMsgDeviceDisplayInfo too large");
> > + } else {
> > + handled = handle_msg_device_display_info(dev, sin);
> > + }
> > + break;
> > case STREAM_TYPE_DATA:
> > if (dev->hdr.size > 32*1024*1024) {
> > handled = handle_msg_invalid(dev, sin, "STREAM_DATA too
> > large");
> > @@ -271,6 +279,58 @@ handle_msg_format(StreamDevice *dev,
> > SpiceCharDeviceInstance *sin)
> > return true;
> > }
> >
> > +static bool
> > +handle_msg_device_display_info(StreamDevice *dev,
> > SpiceCharDeviceInstance *sin)
> > +{
> > + SpiceCharDeviceInterface *sif =
> > spice_char_device_get_interface(sin);
> > +
> > + if (spice_extra_checks) {
> > + spice_assert(dev->hdr_pos >= sizeof(StreamDevHeader));
> > + spice_assert(dev->hdr.type ==
> > STREAM_TYPE_DEVICE_DISPLAY_INFO);
> > + }
> > +
> > + if (dev->msg_len < dev->hdr.size) {
> > + dev->msg = g_realloc(dev->msg, dev->hdr.size);
> > + dev->msg_len = dev->hdr.size;
> > + }
> > +
> > + /* read from device */
> > + ssize_t n = sif->read(sin, dev->msg->buf + dev->msg_pos, dev-
> > > hdr.size - dev->msg_pos);
> >
> > + if (n <= 0) {
> > + return dev->msg_pos == dev->hdr.size;
> > + }
> > +
> > + dev->msg_pos += n;
> > + if (dev->msg_pos != dev->hdr.size) { /* some bytes are still
> > missing */
> > + return false;
> > + }
> > +
> > + StreamMsgDeviceDisplayInfo *display_info_msg = &dev->msg-
> > > device_display_info;
> >
> > +
> > + size_t device_address_len = display_info_msg-
> > > device_address_len;
> >
> > + if (device_address_len > MAX_DEVICE_ADDRESS_LEN) {
> > + g_error("Received a device address longer than %u (%lu), "
> > + "will be truncated!", MAX_DEVICE_ADDRESS_LEN,
> > device_address_len);
> > + device_address_len = sizeof(dev-
> > > device_display_info.device_address);
> >
> > + }
> > +
> > + strncpy(dev->device_display_info.device_address,
> > + (char*) display_info_msg->device_address,
> > + device_address_len);
> > +
> > + dev->device_display_info.device_address[device_address_len] =
> > '\0'; // terminate the string
> > + dev->device_display_info.id = display_info_msg->id;
> > + dev->device_display_info.device_display_id = display_info_msg-
> > > device_display_id;
> >
> > +
> > + g_debug("Received DeviceDisplayInfo from the streaming agent: id
> > %u, "
> > + "device_address %s, device_display_id %u",
> > + dev->device_display_info.id,
> > + dev->device_display_info.device_address,
> > + dev->device_display_info.device_display_id);
> > +
> > + return true;
> > +}
> > +
> > static bool
> > handle_msg_capabilities(StreamDevice *dev, SpiceCharDeviceInstance
> > *sin)
> > {
> > diff --git a/server/red-stream-device.h b/server/red-stream-device.h
> > index f0a5b5c1..9e71cb88 100644
> > --- a/server/red-stream-device.h
> > +++ b/server/red-stream-device.h
> > @@ -19,6 +19,7 @@
> > #ifndef STREAM_DEVICE_H
> > #define STREAM_DEVICE_H
> >
> > +#include "reds.h"
> > #include "char-device.h"
> >
> > G_BEGIN_DECLS
> > @@ -41,6 +42,13 @@ typedef struct StreamDeviceClass
> > StreamDeviceClass;
> > (G_TYPE_INSTANCE_GET_CLASS((obj), TYPE_STREAM_DEVICE,
> > StreamDeviceClass))
> >
> > GType stream_device_get_type(void) G_GNUC_CONST;
> > +
> > +typedef struct StreamDeviceDisplayInfo {
> > + uint32_t id;
> > + char device_address[MAX_DEVICE_ADDRESS_LEN];
> > + uint32_t device_display_id;
> > +} StreamDeviceDisplayInfo;
> > +
> > StreamDevice *stream_device_connect(RedsState *reds,
> > SpiceCharDeviceInstance *sin);
> >
> > /* Create channel for the streaming device.
> > diff --git a/server/reds.h b/server/reds.h
> > index 8260ab5c..675e10d5 100644
> > --- a/server/reds.h
> > +++ b/server/reds.h
> > @@ -30,6 +30,9 @@
> > #include "main-dispatcher.h"
> > #include "migration-protocol.h"
> >
> > +
> > +#define MAX_DEVICE_ADDRESS_LEN 256
> > +
> > static inline QXLInterface * qxl_get_interface(QXLInstance *qxl)
> > {
> > return SPICE_CONTAINEROF(qxl->base.sif, QXLInterface, base);
>
>
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel