date:20180320

Re: [Spice-devel] [PATCH spice-server 2/2] stream-device: Handle capabilities

2018-03-20 Thread Uri Lublin


On 03/19/2018 06:46 PM, Frediano Ziglio wrote:

Handle capabilities from guest device.
Send capability to the guest when device is opened.
Currently there's no capabilities set on the message sent.
On the tests we need to discard the capability message before
reading the error.

Signed-off-by: Frediano Ziglio 
---
  server/red-stream-device.c| 66 +--
  server/tests/test-stream-device.c | 22 +
  2 files changed, 85 insertions(+), 3 deletions(-)

Changes since v1:
- rebased on master (with minor fix due to rename).

diff --git a/server/red-stream-device.c b/server/red-stream-device.c
index e91df88d..1732b888 100644
--- a/server/red-stream-device.c
+++ b/server/red-stream-device.c
@@ -1,6 +1,6 @@
  /* spice-server character device to handle a video stream
  
-   Copyright (C) 2017 Red Hat, Inc.

+   Copyright (C) 2017-2018 Red Hat, Inc.
  
 This library is free software; you can redistribute it and/or

 modify it under the terms of the GNU Lesser General Public
@@ -25,6 +25,8 @@
  #include "cursor-channel.h"
  #include "reds.h"
  
+#define MAX_GUEST_CAPABILITIES_BYTES ((STREAM_CAP_END+7)/8)

+
  struct StreamDevice {
  RedCharDevice parent;
  
@@ -42,6 +44,7 @@ struct StreamDevice {

  bool has_error;
  bool opened;
  bool flow_stopped;
+uint8_t guest_capabilities[MAX_GUEST_CAPABILITIES_BYTES];
  StreamChannel *stream_channel;
  CursorChannel *cursor_channel;
  SpiceTimer *close_timer;
@@ -61,7 +64,7 @@ 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_cursor_move, handle_msg_capabilities;
  
  static bool handle_msg_invalid(StreamDevice *dev, SpiceCharDeviceInstance *sin,

 const char *error_msg) 
SPICE_GNUC_WARN_UNUSED_RESULT;
@@ -147,7 +150,8 @@ stream_device_partial_read(StreamDevice *dev, 
SpiceCharDeviceInstance *sin)
  }
  break;
  case STREAM_TYPE_CAPABILITIES:
-/* FIXME */
+handled = handle_msg_capabilities(dev, sin);
+break;
  default:
  handled = handle_msg_invalid(dev, sin, "Invalid message type");
  break;
@@ -254,6 +258,38 @@ handle_msg_format(StreamDevice *dev, 
SpiceCharDeviceInstance *sin)
  return true;
  }
  
+static bool

+handle_msg_capabilities(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_CAPABILITIES);
+}
+
+if (dev->hdr.size > STREAM_MSG_CAPABILITIES_MAX_BYTES) {

(*)

+return handle_msg_invalid(dev, sin, "Wrong size for 
StreamMsgCapabilities") > +}
+
+int n = sif->read(sin, dev->msg->buf + dev->msg_pos, dev->hdr.size - 
dev->msg_pos);
+if (n < 0) {
+return handle_msg_invalid(dev, sin, NULL);
+}
+
+dev->msg_pos += n;
+
+if (dev->msg_pos < dev->hdr.size) {
+return false;
+}
+
+// copy only capabilities we care about
+memset(dev->guest_capabilities, 0, sizeof(dev->guest_capabilities));
+memcpy(dev->guest_capabilities, dev->msg->buf, 
MIN(MAX_GUEST_CAPABILITIES_BYTES, dev->hdr.size));


nits:
1. If the this line is reached (see *), there is no need for MIN as
   dev->hdr.size <= MAX_GUEST_CAPABILITIES_BYTES

2. All other messages got a structure in spice-protocol.
   This message can have one too (similar to StreamMsgData)

Uri.


+
+return true;
+}
+
  static bool
  handle_msg_data(StreamDevice *dev, SpiceCharDeviceInstance *sin)
  {
@@ -586,6 +622,28 @@ char_device_set_state(RedCharDevice *char_dev, int state)
  }
  }
  
+static void

+send_capabilities(RedCharDevice *char_dev)
+{
+int msg_size = MAX_GUEST_CAPABILITIES_BYTES;
+int total_size = sizeof(StreamDevHeader) + msg_size;
+
+RedCharDeviceWriteBuffer *buf =
+red_char_device_write_buffer_get_server_no_token(char_dev, total_size);
+buf->buf_used = total_size;
+
+StreamDevHeader *const hdr = (StreamDevHeader *)buf->buf;
+hdr->protocol_version = STREAM_DEVICE_PROTOCOL;
+hdr->padding = 0;
+hdr->type = GUINT16_TO_LE(STREAM_TYPE_CAPABILITIES);
+hdr->size = GUINT32_TO_LE(msg_size);
+
+StreamMsgCapabilities *const caps = (StreamMsgCapabilities *)(hdr+1);
+memset(caps, 0, msg_size);
+
+red_char_device_write_buffer_add(char_dev, buf);
+}
+
  static void
  stream_device_port_event(RedCharDevice *char_dev, uint8_t event)
  {
@@ -599,6 +657,8 @@ stream_device_port_event(RedCharDevice *char_dev, uint8_t 
event)
  dev->opened = (event == SPICE_PORT_EVENT_OPENED);
  if (dev->opened) {
  stream_device_create_channel(dev);
+
+send_capabilities(char_dev);

Re: [Spice-devel] [PATCH spice-server 2/2] stream-device: Handle capabilities

2018-03-20 Thread Uri Lublin


On 03/20/2018 01:28 PM, Frediano Ziglio wrote:


Looks good, with minor nits.


On 19 Mar 2018, at 17:46, Frediano Ziglio  wrote:

Handle capabilities from guest device.
Send capability to the guest when device is opened.
Currently there's no capabilities set on the message sent.
On the tests we need to discard the capability message before
reading the error.

Signed-off-by: Frediano Ziglio 
---
server/red-stream-device.c| 66
+--
server/tests/test-stream-device.c | 22 +
2 files changed, 85 insertions(+), 3 deletions(-)

Changes since v1:
- rebased on master (with minor fix due to rename).

diff --git a/server/red-stream-device.c b/server/red-stream-device.c
index e91df88d..1732b888 100644
--- a/server/red-stream-device.c
+++ b/server/red-stream-device.c
@@ -1,6 +1,6 @@
/* spice-server character device to handle a video stream

-   Copyright (C) 2017 Red Hat, Inc.
+   Copyright (C) 2017-2018 Red Hat, Inc.

This library is free software; you can redistribute it and/or
modify it under the terms of the GNU Lesser General Public
@@ -25,6 +25,8 @@
#include "cursor-channel.h"
#include "reds.h"

+#define MAX_GUEST_CAPABILITIES_BYTES ((STREAM_CAP_END+7)/8)


Currently no capability is defined, so:
  STREAM_CAP_END = MAX_GUEST_CAPABILITIES_BYTES = 0


+
struct StreamDevice {
 RedCharDevice parent;

@@ -42,6 +44,7 @@ struct StreamDevice {
 bool has_error;
 bool opened;
 bool flow_stopped;
+uint8_t guest_capabilities[MAX_GUEST_CAPABILITIES_BYTES];
 StreamChannel *stream_channel;
 CursorChannel *cursor_channel;
 SpiceTimer *close_timer;


...


@@ -254,6 +258,38 @@ handle_msg_format(StreamDevice *dev,
SpiceCharDeviceInstance *sin)
 return true;
}

+static bool
+handle_msg_capabilities(StreamDevice *dev, SpiceCharDeviceInstance *sin)
+{
+SpiceCharDeviceInterface *sif = spice_char_device_get_interface(sin);
+
+if (spice_extra_checks) {


Premature optimization.



Extra is not expensive and code is coherent with other part.


+spice_assert(dev->hdr_pos >= sizeof(StreamDevHeader));
+spice_assert(dev->hdr.type == STREAM_TYPE_CAPABILITIES);
+}
+
+if (dev->hdr.size > STREAM_MSG_CAPABILITIES_MAX_BYTES) {
+return handle_msg_invalid(dev, sin, "Wrong size for
StreamMsgCapabilities");
+}
+
+int n = sif->read(sin, dev->msg->buf + dev->msg_pos, dev->hdr.size -
dev->msg_pos);
+if (n < 0) {


Reading the various sif->read, the convention on return values is a bit
unclear. Most other places seem to check for <= 0. Only handle_msg_format
uses < 0. Someone could teach me why?

Is it possible for sif->read to return 0 on error?


No, 0 is 0 byte in the current buffer which does not mean end of file,
there's no EAGAIN behaviour.
Basically with <=0 you handle either 0 bytes or error while with <0 only
errors.


Is it possible for sif->read to return less than the requested size (e.g.
EINTR)?



There's no EINTR but what can happen is that guest did a partial write
or the buffer was full so the write was truncated. The interface is not
blocking so partial read are normal.



Since, currently, there are no capabilities
dev->hdr.size - dev->msg_pos = 0 - 0 = 0

The call sif->read(sin,buffer,0) returns 0.

Uri
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

[Spice-devel] [spice-vdagnet (linux) PATCH] x11-randr: do not assume each output has ncrtc=1

2018-03-20 Thread Uri Lublin

This was true for virtual graphic cards, but not true for
device-assigned graphic cards.
For example NVIDIA M2000 has 8.

Still we currently pick only a single crtc for each output
(but looping over them to find an active one)

Signed-off-by: Uri Lublin 
---
 src/vdagent/x11-randr.c | 15 +++
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/src/vdagent/x11-randr.c b/src/vdagent/x11-randr.c
index aade5ca..0c196de 100644
--- a/src/vdagent/x11-randr.c
+++ b/src/vdagent/x11-randr.c
@@ -58,6 +58,8 @@ static XRRCrtcInfo *crtc_from_id(struct vdagent_x11 *x11, int 
id)
 {
 int i;
 
+if (id == 0)
+return NULL;
 for (i = 0 ; i < x11->randr.res->ncrtc ; ++i) {
 if (id == x11->randr.res->crtcs[i]) {
 return x11->randr.crtcs[i];
@@ -650,7 +652,7 @@ static int config_size(int num_of_monitors)
 
 static VDAgentMonitorsConfig *get_current_mon_config(struct vdagent_x11 *x11)
 {
-int i, num_of_monitors = 0;
+int i, j, num_of_monitors = 0;
 XRRModeInfo *mode;
 XRRCrtcInfo *crtc;
 XRRScreenResources *res = x11->randr.res;
@@ -665,10 +667,15 @@ static VDAgentMonitorsConfig 
*get_current_mon_config(struct vdagent_x11 *x11)
 for (i = 0 ; i < res->noutput; i++) {
 if (x11->randr.outputs[i]->ncrtc == 0)
 continue; /* Monitor disabled, already zero-ed by calloc */
-if (x11->randr.outputs[i]->ncrtc != 1)
-goto error;
+if (x11->randr.outputs[i]->crtc == 0)
+continue; /* Monitor disabled */
 
-crtc = crtc_from_id(x11, x11->randr.outputs[i]->crtcs[0]);
+for (j=0; jrandr.outputs[i]->ncrtc; j++) {
+crtc = crtc_from_id(x11, x11->randr.outputs[i]->crtcs[j]);
+if (crtc) {
+break;
+}
+}
 if (!crtc)
 goto error;
 
-- 
2.14.3

___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH spice-server 2/2] stream-device: Handle capabilities

2018-03-20 Thread Frediano Ziglio

> 
> Looks good, with minor nits.
> 
> > On 19 Mar 2018, at 17:46, Frediano Ziglio  wrote:
> > 
> > Handle capabilities from guest device.
> > Send capability to the guest when device is opened.
> > Currently there's no capabilities set on the message sent.
> > On the tests we need to discard the capability message before
> > reading the error.
> > 
> > Signed-off-by: Frediano Ziglio 
> > ---
> > server/red-stream-device.c| 66
> > +--
> > server/tests/test-stream-device.c | 22 +
> > 2 files changed, 85 insertions(+), 3 deletions(-)
> > 
> > Changes since v1:
> > - rebased on master (with minor fix due to rename).
> > 
> > diff --git a/server/red-stream-device.c b/server/red-stream-device.c
> > index e91df88d..1732b888 100644
> > --- a/server/red-stream-device.c
> > +++ b/server/red-stream-device.c
> > @@ -1,6 +1,6 @@
> > /* spice-server character device to handle a video stream
> > 
> > -   Copyright (C) 2017 Red Hat, Inc.
> > +   Copyright (C) 2017-2018 Red Hat, Inc.
> > 
> >This library is free software; you can redistribute it and/or
> >modify it under the terms of the GNU Lesser General Public
> > @@ -25,6 +25,8 @@
> > #include "cursor-channel.h"
> > #include "reds.h"
> > 
> > +#define MAX_GUEST_CAPABILITIES_BYTES ((STREAM_CAP_END+7)/8)
> > +
> > struct StreamDevice {
> > RedCharDevice parent;
> > 
> > @@ -42,6 +44,7 @@ struct StreamDevice {
> > bool has_error;
> > bool opened;
> > bool flow_stopped;
> > +uint8_t guest_capabilities[MAX_GUEST_CAPABILITIES_BYTES];
> > StreamChannel *stream_channel;
> > CursorChannel *cursor_channel;
> > SpiceTimer *close_timer;
> > @@ -61,7 +64,7 @@ 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_cursor_move, handle_msg_capabilities;
> > 
> > static bool handle_msg_invalid(StreamDevice *dev, SpiceCharDeviceInstance
> > *sin,
> >const char *error_msg)
> >SPICE_GNUC_WARN_UNUSED_RESULT;
> > @@ -147,7 +150,8 @@ stream_device_partial_read(StreamDevice *dev,
> > SpiceCharDeviceInstance *sin)
> > }
> > break;
> > case STREAM_TYPE_CAPABILITIES:
> > -/* FIXME */
> > +handled = handle_msg_capabilities(dev, sin);
> > +break;
> > default:
> > handled = handle_msg_invalid(dev, sin, "Invalid message type");
> > break;
> > @@ -254,6 +258,38 @@ handle_msg_format(StreamDevice *dev,
> > SpiceCharDeviceInstance *sin)
> > return true;
> > }
> > 
> > +static bool
> > +handle_msg_capabilities(StreamDevice *dev, SpiceCharDeviceInstance *sin)
> > +{
> > +SpiceCharDeviceInterface *sif = spice_char_device_get_interface(sin);
> > +
> > +if (spice_extra_checks) {
> 
> Premature optimization.
> 

Extra is not expensive and code is coherent with other part.

> > +spice_assert(dev->hdr_pos >= sizeof(StreamDevHeader));
> > +spice_assert(dev->hdr.type == STREAM_TYPE_CAPABILITIES);
> > +}
> > +
> > +if (dev->hdr.size > STREAM_MSG_CAPABILITIES_MAX_BYTES) {
> > +return handle_msg_invalid(dev, sin, "Wrong size for
> > StreamMsgCapabilities");
> > +}
> > +
> > +int n = sif->read(sin, dev->msg->buf + dev->msg_pos, dev->hdr.size -
> > dev->msg_pos);
> > +if (n < 0) {
> 
> Reading the various sif->read, the convention on return values is a bit
> unclear. Most other places seem to check for <= 0. Only handle_msg_format
> uses < 0. Someone could teach me why?
> 
> Is it possible for sif->read to return 0 on error?

No, 0 is 0 byte in the current buffer which does not mean end of file,
there's no EAGAIN behaviour.
Basically with <=0 you handle either 0 bytes or error while with <0 only
errors.

> Is it possible for sif->read to return less than the requested size (e.g.
> EINTR)?
> 

There's no EINTR but what can happen is that guest did a partial write
or the buffer was full so the write was truncated. The interface is not
blocking so partial read are normal.

> 
> > +return handle_msg_invalid(dev, sin, NULL);
> > +}
> > +
> > +dev->msg_pos += n;
> > +
> > +if (dev->msg_pos < dev->hdr.size) {
> > +return false;
> > +}
> > +
> > +// copy only capabilities we care about
> 
> I think it’s “capabilities we know about”, since ifsd we get extra, it’s
> probably stuff added after this version.
> 

updated

> > +memset(dev->guest_capabilities, 0, sizeof(dev->guest_capabilities));
> > +memcpy(dev->guest_capabilities, dev->msg->buf,
> > MIN(MAX_GUEST_CAPABILITIES_BYTES, dev->hdr.size));
> > +
> > +return true;
> > +}
> > +
> > static bool
> > handle_msg_data(StreamDevice *dev, SpiceCharDeviceInstance *sin)
> > {
> > @@ -586,6 +622,28 @@

Re: [Spice-devel] [PATCH spice-server] stream-channel: Implements monitors_config

2018-03-20 Thread Frediano Ziglio

> > On 19 Mar 2018, at 11:39, Frediano Ziglio  wrote:
> > 
> >> 
> >> On 03/13/2018 08:21 AM, Frediano Ziglio wrote:
> >>> Although not necessary for a single monitor DisplayChannel implementation
> >>> this make the DiisplayChannels more coherent from the client
> >>> point of view.
> >>> 
> >>> Signed-off-by: Frediano Ziglio 
> >>> ---
> >>>  server/stream-channel.c | 33 ++---
> >>>  1 file changed, 30 insertions(+), 3 deletions(-)
> >>> 
> >>> diff --git a/server/stream-channel.c b/server/stream-channel.c
> >>> index 3a3b733f..a9882d9c 100644
> >>> --- a/server/stream-channel.c
> >>> +++ b/server/stream-channel.c
> >>> @@ -102,6 +102,7 @@ enum {
> >>>  RED_PIPE_ITEM_TYPE_STREAM_DATA,
> >>>  RED_PIPE_ITEM_TYPE_STREAM_DESTROY,
> >>>  RED_PIPE_ITEM_TYPE_STREAM_ACTIVATE_REPORT,
> >>> +RED_PIPE_ITEM_TYPE_MONITORS_CONFIG,
> >>>  };
> >>> 
> >>>  typedef struct StreamCreateItem {
> >>> @@ -204,6 +205,26 @@ fill_base(SpiceMarshaller *m, const StreamChannel
> >>> *channel)
> >>>  spice_marshall_DisplayBase(m, );
> >>>  }
> >>> 
> >>> +static void
> >>> +marshall_monitors_config(RedChannelClient *rcc, StreamChannel *channel,
> >>> SpiceMarshaller *m)
> >>> +{
> >>> +struct {
> >>> +SpiceMsgDisplayMonitorsConfig config;
> >>> +SpiceHead head;
> >>> +} msg = {
> >>> +{ 1, 1, },
> >>> +{
> >>> +0, PRIMARY_SURFACE_ID,
> >> 
> >> Hi Frediano,
> >> 
> >> Why do you send id=0 ?
> >> 
> >> Uri.
> >> 
> > 
> > Too much IDs :-)
> > These IDs are allocated starting from 0 and are per channel so as there
> > is only a monitor (currently) the ID should be 0.
> > So this id should not be confused with QXL ID or client/guest ID.
> 
> If Uri is confused, a comment may be in order.
> 
> Why so many IDs? Is it just to avoid an ID lookup during initialization?
> 

Not getting the lookup.

We have a global idea of the "monitor ID" which is quite confusing and
prone to different issues. The DisplayChannel (protocol) use channel IDs
(allocated per channel type starting from 0) and head IDs (allocated per
channel starting from 0). The client and vdagent use the global monitor ID.
The client craft these monitor IDs using basically the formula
"channel ID ? channel ID : head ID" and assuming the vdagent uses the same
IDs. Currently there are different problems of these assumption/schema.

> > 
> > Frediano
> > 
> >>> +channel->width, channel->height,
> >>> +0, 0,
> >>> +0 // flags
> >>> +}
> >>> +};
> >>> +
> >>> +red_channel_client_init_send_data(rcc,
> >>> SPICE_MSG_DISPLAY_MONITORS_CONFIG);
> >>> +spice_marshall_msg_display_monitors_config(m, );
> >>> +}
> >>> +
> >>>  static void
> >>>  stream_channel_send_item(RedChannelClient *rcc, RedPipeItem *pipe_item)
> >>>  {
> >>> @@ -229,6 +250,12 @@ stream_channel_send_item(RedChannelClient *rcc,
> >>> RedPipeItem *pipe_item)
> >>>  spice_marshall_msg_display_surface_create(m, _create);
> >>>  break;
> >>>  }
> >>> +case RED_PIPE_ITEM_TYPE_MONITORS_CONFIG:
> >>> +if (!red_channel_client_test_remote_cap(rcc,
> >>> SPICE_DISPLAY_CAP_MONITORS_CONFIG)) {
> >>> +return;
> >>> +}
> >>> +marshall_monitors_config(rcc, channel, m);
> >>> +break;
> >>>  case RED_PIPE_ITEM_TYPE_SURFACE_DESTROY: {
> >>>  red_channel_client_init_send_data(rcc,
> >>>  SPICE_MSG_DISPLAY_SURFACE_DESTROY);
> >>>  SpiceMsgSurfaceDestroy surface_destroy = { PRIMARY_SURFACE_ID };
> >>> @@ -397,7 +424,6 @@ stream_channel_connect(RedChannel *red_channel,
> >>> RedClient *red_client, RedStream
> >>>  request_new_stream(channel, start);
> >>> 
> >>> 
> >>> -// TODO set capabilities like  SPICE_DISPLAY_CAP_MONITORS_CONFIG
> >>>  // see guest_set_client_capabilities
> >>>  RedChannelClient *rcc = RED_CHANNEL_CLIENT(client);
> >>>  red_channel_client_push_set_ack(rcc);
> >>> @@ -415,6 +441,7 @@ stream_channel_connect(RedChannel *red_channel,
> >>> RedClient *red_client, RedStream
> >>> 
> >>>  // pass proper data
> >>>  red_channel_client_pipe_add_type(rcc,
> >>>  RED_PIPE_ITEM_TYPE_SURFACE_CREATE);
> >>> +red_channel_client_pipe_add_type(rcc,
> >>> RED_PIPE_ITEM_TYPE_MONITORS_CONFIG);
> >>>  // surface data
> >>>  red_channel_client_pipe_add_type(rcc,
> >>>  RED_PIPE_ITEM_TYPE_FILL_SURFACE);
> >>>  // TODO monitor configs ??
> >>> @@ -433,8 +460,7 @@ stream_channel_constructed(GObject *object)
> >>>  client_cbs.connect = stream_channel_connect;
> >>>  red_channel_register_client_cbs(red_channel, _cbs, NULL);
> >>> 
> >>> -// TODO, send monitor to support multiple monitors in the future
> >>> -//red_channel_set_cap(red_channel,
> >>> SPICE_DISPLAY_CAP_MONITORS_CONFIG);
> >>> +red_channel_set_cap(red_channel, SPICE_DISPLAY_CAP_MONITORS_CONFIG);
> >>>

Re: [Spice-devel] [PATCH spice-common 1/2] Add --enable-extra-checks option

2018-03-20 Thread Frediano Ziglio

> > On 19 Mar 2018, at 14:46, Frediano Ziglio  wrote:
> > 
> > Allow to enable code to do additional or expensive checks.
> 
> “Allow to enable…” -> “Add configuration option enabling expensive checks”
> 

We decided extra after researching on different projects.
I think extra is better, not only expensive but also paranoid tests
I would say that you don't want on final product.

> > The option should be used by higher level libraries.
> > By default the option is disabled.
> > 
> > Signed-off-by: Frediano Ziglio 
> > ---
> > common/log.h |  6 ++
> > configure.ac |  1 +
> > m4/spice-deps.m4 | 14 ++
> > 3 files changed, 21 insertions(+)
> > 
> > diff --git a/common/log.h b/common/log.h
> > index 06d48d2..e0fd34b 100644
> > --- a/common/log.h
> > +++ b/common/log.h
> > @@ -93,6 +93,12 @@ void spice_log(GLogLevelFlags log_level,
> > }   \
> > } G_STMT_END
> > 
> > +#if ENABLE_EXTRA_CHECKS
> > +enum { spice_extra_checks = 1 };
> 
> I would suggest renaming “extra_checks” to “expensive_checks” if that’s
> really the intent.

No, is extra.

> And then, I would avoid bracketing inexpensive checks (like simple asserts)
> with it, because otherwise it introduces confusion.
> 

I don't see why an if is confusing. They are "extra", not "expensive".

> Alternatively, if we think that asserts are expensive, then all spice_assert
> checks should be disabled by this. But I’d rather not.
> 
> (I already suggested a spice_hot_assert for assertions in hot code as a
> better way to express the intent than an if around spice_assert).
> 

I don't agree on a rule like "is expensive disable the assert", depends
on the probability of happening and the problem that can raise not doing
it. I prefer a core than a remote code execution, but here we enter on
the opinions.

> > +#else
> > +enum { spice_extra_checks = 0 };
> > +#endif
> > +
> > SPICE_END_DECLS
> > 
> > #endif /* H_SPICE_LOG */
> > diff --git a/configure.ac b/configure.ac
> > index 3542161..3da85de 100644
> > --- a/configure.ac
> > +++ b/configure.ac
> > @@ -27,6 +27,7 @@ fi
> > AM_PROG_CC_C_O
> > 
> > SPICE_CHECK_SYSDEPS
> > +SPICE_EXTRA_CHECKS
> > 
> > AC_ARG_ENABLE([alignment-checks],
> >   AS_HELP_STRING([--enable-alignment-checks],
> > diff --git a/m4/spice-deps.m4 b/m4/spice-deps.m4
> > index 68e3091..a6f4b7b 100644
> > --- a/m4/spice-deps.m4
> > +++ b/m4/spice-deps.m4
> > @@ -23,6 +23,20 @@ AC_DEFUN([SPICE_PRINT_MESSAGES],[
> > ])
> > 
> > 
> > +# SPICE_EXTRA_CHECKS()
> > +# 
> > +# Check for --enable-extra-checks option
> > +# 
> > +AC_DEFUN([SPICE_EXTRA_CHECKS],[
> > +AC_ARG_ENABLE([extra-checks],
> > +   AS_HELP_STRING([--enable-extra-checks=@<:@yes/no@:>@],
> > +  [Enable expensive checks
> > @<:@default=no@:>@]))
> > +AM_CONDITIONAL(ENABLE_EXTRA_CHECKS, test "x$enable_extra_checks" = "xyes")
> > +AS_IF([test "x$enable_extra_checks" = "xyes"],
> > +  [AC_DEFINE([ENABLE_EXTRA_CHECKS], 1, [Enable extra checks on
> > code])])
> > +])
> > +
> > +
> > # SPICE_CHECK_SYSDEPS()
> > # -
> > # Checks for header files and library functions needed by spice-common.

Here comments would be better using "dnl" style but I used shell comments
for coherence.

Frediano
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH spice-common 3/3] Avoid integer overflow computing image sizes

2018-03-20 Thread Frediano Ziglio

> 
> If you have declared right types from the previous patch, is this really
> necessary?
> 

Previous patch affects other code paths.

> > On 19 Mar 2018, at 11:06, Frediano Ziglio  wrote:
> > 
> > Use always 64, sizes can be 32x32.
> > 
> > Signed-off-by: Frediano Ziglio 
> > ---
> > python_modules/demarshal.py | 14 ++
> > python_modules/marshal.py   |  7 +++
> > 2 files changed, 9 insertions(+), 12 deletions(-)
> > 
> > diff --git a/python_modules/demarshal.py b/python_modules/demarshal.py
> > index 7e73985..8d3f5cb 100644
> > --- a/python_modules/demarshal.py
> > +++ b/python_modules/demarshal.py
> > @@ -346,13 +346,12 @@ def write_validate_array_item(writer, container,
> > item, scope, parent_scope, star
> > rows = array.size[3]
> > width_v = write_read_primitive(writer, start, container, width,
> > scope)
> > rows_v = write_read_primitive(writer, start, container, rows,
> > scope)
> > -# TODO: Handle multiplication overflow
> > if bpp == 8:
> > -writer.assign(nelements, "%s * %s" % (width_v, rows_v))
> > +writer.assign(nelements, "(uint64_t) %s * %s" % (width_v,
> > rows_v))
> > elif bpp == 1:
> > -writer.assign(nelements, "((%s + 7) / 8 ) * %s" % (width_v,
> > rows_v))
> > +writer.assign(nelements, "(((uint64_t) %s + 7U) / 8U ) * %s" %
> > (width_v, rows_v))
> > else:
> > -writer.assign(nelements, "((%s * %s + 7) / 8 ) * %s" % (bpp,
> > width_v, rows_v))
> > +writer.assign(nelements, "((%sU * (uint64_t) %s + 7U) / 8U ) *
> > %s" % (bpp, width_v, rows_v))
> > elif array.is_bytes_length():
> > is_byte_size = True
> > v = write_read_primitive(writer, start, container, array.size[1],
> > scope)
> > @@ -713,13 +712,12 @@ def read_array_len(writer, prefix, array, dest,
> > scope, is_ptr):
> > rows = array.size[3]
> > width_v = dest.get_ref(width)
> > rows_v = dest.get_ref(rows)
> > -# TODO: Handle multiplication overflow
> > if bpp == 8:
> > -writer.assign(nelements, "%s * %s" % (width_v, rows_v))
> > +writer.assign(nelements, "((uint64_t) %s * %s)" % (width_v,
> > rows_v))
> > elif bpp == 1:
> > -writer.assign(nelements, "((%s + 7) / 8 ) * %s" % (width_v,
> > rows_v))
> > +writer.assign(nelements, "(((uint64_t) %s + 7U) / 8U ) * %s" %
> > (width_v, rows_v))
> > else:
> > -writer.assign(nelements, "((%s * %s + 7) / 8 ) * %s" % (bpp,
> > width_v, rows_v))
> > +writer.assign(nelements, "((%sU * (uint64_t) %s + 7U) / 8U ) *
> > %s" % (bpp, width_v, rows_v))
> > elif array.is_bytes_length():
> > writer.assign(nelements, dest.get_ref(array.size[2]))
> > else:
> > diff --git a/python_modules/marshal.py b/python_modules/marshal.py
> > index 402273c..fd3416a 100644
> > --- a/python_modules/marshal.py
> > +++ b/python_modules/marshal.py
> > @@ -172,13 +172,12 @@ def get_array_size(array, container_src):
> > rows = array.size[3]
> > width_v = container_src.get_ref(width)
> > rows_v = container_src.get_ref(rows)
> > -# TODO: Handle multiplication overflow
> > if bpp == 8:
> > -return "(unsigned) (%s * %s)" % (width_v, rows_v)
> > +return "((uint64_t) %s * %s)" % (width_v, rows_v)
> > elif bpp == 1:
> > -return "(unsigned) (((%s + 7) / 8 ) * %s)" % (width_v, rows_v)
> > +return "uint64_t) %s + 7U) / 8U ) * %s)" % (width_v,
> > rows_v)
> > else:
> > -return "(unsigned) (((%s * %s + 7) / 8 ) * %s)" % (bpp,
> > width_v, rows_v)
> > +return "uint64_t) %s * %s + 7U) / 8U ) * %s)" % (bpp,
> > width_v, rows_v)
> > elif array.is_bytes_length():
> > return container_src.get_ref(array.size[2])
> > else:

Frediano
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH spice-common 1/3] Fix integer overflows computing sizes

2018-03-20 Thread Frediano Ziglio

> 
> > On 19 Mar 2018, at 11:06, Frediano Ziglio  wrote:
> > 
> > Make code safe using both 32 and 64 bit machine.
> > Consider that this code can be compiled for machines with 32 bit.
> > There are some arrays length which are 32 bit.
> > 
> > If size_t this can cause easily an overflow. For instance message_len
> > sending SPICE_MSG_NOTIFY messages are 32 bit and code add a small
> > constant (currently 24) before doing the test for size. Now passing
> > (uint32_t) -20 as message_len would lead to a size of 4 after the
> > addition. This overflow does not happen on 64 bit machine as the length
> > is converted to size_t.
> 
> Why not use size_t instead of uint64_t then?
> 

A multiplication between 32 bit integer and a 32 bit integer can
cause overflow if the result is a 32 bit. Using a 64 bit integer
to multiply a 32 bit integer by a 32 bit integer avoids the overflow.
On 32 bit systems usually size_t is a 32 bit.

> > 
> > There are also some array length where some item are bigger than 1 byte.
> > For instance SPICE_MAIN_CHANNELS_LIST message have a number of channels
> > and each channel is composed by 2 bytes. Now the code generated try to do
> > length * 2 where length is still a 32 bit so if we put a value like
> > 0x8002u we get 4 as length. This will cause an overflow as code will
> > allocate very few bytes but try to fill with a huge number of elements.
> > This overflow happen in both 32 and 64 bit machine.
> > 
> > To avoid all these possible overflows this patch use only 64 bit for
> > nelements (number of elements), nw_size (network size) and mem_size
> > (memory size needed) checking the sizes to avoid other overflows
> > (like pointers conversions under 32 bit machines).
> > 
> > Signed-off-by: Frediano Ziglio 
> > ---
> > python_modules/demarshal.py | 38 +++---
> > 1 file changed, 19 insertions(+), 19 deletions(-)
> > 
> > diff --git a/python_modules/demarshal.py b/python_modules/demarshal.py
> > index da87d44..7e73985 100644
> > --- a/python_modules/demarshal.py
> > +++ b/python_modules/demarshal.py
> > @@ -101,7 +101,7 @@ def write_parser_helpers(writer):
> > writer.variable_def("uint64_t", "offset")
> > writer.variable_def("parse_func_t", "parse")
> > writer.variable_def("void **", "dest")
> > -writer.variable_def("uint32_t", "nelements")
> > +writer.variable_def("uint64_t", "nelements")
> > writer.end_block(semicolon=True)
> > 
> > def write_read_primitive(writer, start, container, name, scope):
> > @@ -186,7 +186,7 @@ def write_validate_switch_member(writer, mprefix,
> > container, switch_member, scop
> > 
> > all_as_extra_size = m.is_extra_size() and want_extra_size
> > if not want_mem_size and all_as_extra_size and not
> > scope.variable_defined(item.mem_size()):
> > -scope.variable_def("uint32_t", item.mem_size())
> > +scope.variable_def("uint64_t", item.mem_size())
> > 
> > sub_want_mem_size = want_mem_size or all_as_extra_size
> > sub_want_extra_size = want_extra_size and not all_as_extra_size
> > @@ -219,7 +219,7 @@ def write_validate_struct_function(writer, struct):
> > scope = writer.function(validate_function, "static intptr_t", "uint8_t
> > *message_start, uint8_t *message_end, uint64_t offset,
> > SPICE_GNUC_UNUSED int minor")
> > scope.variable_def("uint8_t *", "start = message_start + offset")
> > scope.variable_def("SPICE_GNUC_UNUSED uint8_t *", "pos")
> > -scope.variable_def("size_t", "mem_size", "nw_size")
> > +scope.variable_def("uint64_t", "mem_size", "nw_size")
> > num_pointers = struct.get_num_pointers()
> > if  num_pointers != 0:
> > scope.variable_def("SPICE_GNUC_UNUSED intptr_t", "ptr_size")
> > @@ -236,7 +236,7 @@ def write_validate_struct_function(writer, struct):
> > 
> > writer.newline()
> > writer.comment("Check if struct fits in reported side").newline()
> > -writer.error_check("start + nw_size > message_end")
> > +writer.error_check("nw_size > (uintptr_t) (message_end - start)")
> > 
> > writer.statement("return mem_size")
> > 
> > @@ -264,26 +264,26 @@ def write_validate_pointer_item(writer, container,
> > item, scope, parent_scope, st
> > # if array, need no function check
> > 
> > if target_type.is_array():
> > -writer.error_check("message_start + %s >= message_end" % v)
> > +writer.error_check("%s >= (uintptr_t) (message_end -
> > message_start)" % v)
> > 
> > 
> > assert target_type.element_type.is_primitive()
> > 
> > array_item = ItemInfo(target_type, "%s__array" % item.prefix,
> > start)
> > -scope.variable_def("uint32_t", array_item.nw_size())
> > +scope.variable_def("uint64_t", array_item.nw_size())
> > # don't create a variable that isn't used, fixes
> >

Re: [Spice-devel] [PATCH spice-common 3/3] Avoid integer overflow computing image sizes

2018-03-20 Thread Christophe de Dinechin

If you have declared right types from the previous patch, is this really 
necessary?

> On 19 Mar 2018, at 11:06, Frediano Ziglio  wrote:
> 
> Use always 64, sizes can be 32x32.
> 
> Signed-off-by: Frediano Ziglio 
> ---
> python_modules/demarshal.py | 14 ++
> python_modules/marshal.py   |  7 +++
> 2 files changed, 9 insertions(+), 12 deletions(-)
> 
> diff --git a/python_modules/demarshal.py b/python_modules/demarshal.py
> index 7e73985..8d3f5cb 100644
> --- a/python_modules/demarshal.py
> +++ b/python_modules/demarshal.py
> @@ -346,13 +346,12 @@ def write_validate_array_item(writer, container, item, 
> scope, parent_scope, star
> rows = array.size[3]
> width_v = write_read_primitive(writer, start, container, width, scope)
> rows_v = write_read_primitive(writer, start, container, rows, scope)
> -# TODO: Handle multiplication overflow
> if bpp == 8:
> -writer.assign(nelements, "%s * %s" % (width_v, rows_v))
> +writer.assign(nelements, "(uint64_t) %s * %s" % (width_v, 
> rows_v))
> elif bpp == 1:
> -writer.assign(nelements, "((%s + 7) / 8 ) * %s" % (width_v, 
> rows_v))
> +writer.assign(nelements, "(((uint64_t) %s + 7U) / 8U ) * %s" % 
> (width_v, rows_v))
> else:
> -writer.assign(nelements, "((%s * %s + 7) / 8 ) * %s" % (bpp, 
> width_v, rows_v))
> +writer.assign(nelements, "((%sU * (uint64_t) %s + 7U) / 8U ) * 
> %s" % (bpp, width_v, rows_v))
> elif array.is_bytes_length():
> is_byte_size = True
> v = write_read_primitive(writer, start, container, array.size[1], 
> scope)
> @@ -713,13 +712,12 @@ def read_array_len(writer, prefix, array, dest, scope, 
> is_ptr):
> rows = array.size[3]
> width_v = dest.get_ref(width)
> rows_v = dest.get_ref(rows)
> -# TODO: Handle multiplication overflow
> if bpp == 8:
> -writer.assign(nelements, "%s * %s" % (width_v, rows_v))
> +writer.assign(nelements, "((uint64_t) %s * %s)" % (width_v, 
> rows_v))
> elif bpp == 1:
> -writer.assign(nelements, "((%s + 7) / 8 ) * %s" % (width_v, 
> rows_v))
> +writer.assign(nelements, "(((uint64_t) %s + 7U) / 8U ) * %s" % 
> (width_v, rows_v))
> else:
> -writer.assign(nelements, "((%s * %s + 7) / 8 ) * %s" % (bpp, 
> width_v, rows_v))
> +writer.assign(nelements, "((%sU * (uint64_t) %s + 7U) / 8U ) * 
> %s" % (bpp, width_v, rows_v))
> elif array.is_bytes_length():
> writer.assign(nelements, dest.get_ref(array.size[2]))
> else:
> diff --git a/python_modules/marshal.py b/python_modules/marshal.py
> index 402273c..fd3416a 100644
> --- a/python_modules/marshal.py
> +++ b/python_modules/marshal.py
> @@ -172,13 +172,12 @@ def get_array_size(array, container_src):
> rows = array.size[3]
> width_v = container_src.get_ref(width)
> rows_v = container_src.get_ref(rows)
> -# TODO: Handle multiplication overflow
> if bpp == 8:
> -return "(unsigned) (%s * %s)" % (width_v, rows_v)
> +return "((uint64_t) %s * %s)" % (width_v, rows_v)
> elif bpp == 1:
> -return "(unsigned) (((%s + 7) / 8 ) * %s)" % (width_v, rows_v)
> +return "uint64_t) %s + 7U) / 8U ) * %s)" % (width_v, rows_v)
> else:
> -return "(unsigned) (((%s * %s + 7) / 8 ) * %s)" % (bpp, width_v, 
> rows_v)
> +return "uint64_t) %s * %s + 7U) / 8U ) * %s)" % (bpp, 
> width_v, rows_v)
> elif array.is_bytes_length():
> return container_src.get_ref(array.size[2])
> else:
> -- 
> 2.14.3
> 
> ___
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel

___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH spice-common 1/3] Fix integer overflows computing sizes

2018-03-20 Thread Christophe de Dinechin



> On 19 Mar 2018, at 11:06, Frediano Ziglio  wrote:
> 
> Make code safe using both 32 and 64 bit machine.
> Consider that this code can be compiled for machines with 32 bit.
> There are some arrays length which are 32 bit.
> 
> If size_t this can cause easily an overflow. For instance message_len
> sending SPICE_MSG_NOTIFY messages are 32 bit and code add a small
> constant (currently 24) before doing the test for size. Now passing
> (uint32_t) -20 as message_len would lead to a size of 4 after the
> addition. This overflow does not happen on 64 bit machine as the length
> is converted to size_t.

Why not use size_t instead of uint64_t then?

> 
> There are also some array length where some item are bigger than 1 byte.
> For instance SPICE_MAIN_CHANNELS_LIST message have a number of channels
> and each channel is composed by 2 bytes. Now the code generated try to do
> length * 2 where length is still a 32 bit so if we put a value like
> 0x8002u we get 4 as length. This will cause an overflow as code will
> allocate very few bytes but try to fill with a huge number of elements.
> This overflow happen in both 32 and 64 bit machine.
> 
> To avoid all these possible overflows this patch use only 64 bit for
> nelements (number of elements), nw_size (network size) and mem_size
> (memory size needed) checking the sizes to avoid other overflows
> (like pointers conversions under 32 bit machines).
> 
> Signed-off-by: Frediano Ziglio 
> ---
> python_modules/demarshal.py | 38 +++---
> 1 file changed, 19 insertions(+), 19 deletions(-)
> 
> diff --git a/python_modules/demarshal.py b/python_modules/demarshal.py
> index da87d44..7e73985 100644
> --- a/python_modules/demarshal.py
> +++ b/python_modules/demarshal.py
> @@ -101,7 +101,7 @@ def write_parser_helpers(writer):
> writer.variable_def("uint64_t", "offset")
> writer.variable_def("parse_func_t", "parse")
> writer.variable_def("void **", "dest")
> -writer.variable_def("uint32_t", "nelements")
> +writer.variable_def("uint64_t", "nelements")
> writer.end_block(semicolon=True)
> 
> def write_read_primitive(writer, start, container, name, scope):
> @@ -186,7 +186,7 @@ def write_validate_switch_member(writer, mprefix, 
> container, switch_member, scop
> 
> all_as_extra_size = m.is_extra_size() and want_extra_size
> if not want_mem_size and all_as_extra_size and not 
> scope.variable_defined(item.mem_size()):
> -scope.variable_def("uint32_t", item.mem_size())
> +scope.variable_def("uint64_t", item.mem_size())
> 
> sub_want_mem_size = want_mem_size or all_as_extra_size
> sub_want_extra_size = want_extra_size and not all_as_extra_size
> @@ -219,7 +219,7 @@ def write_validate_struct_function(writer, struct):
> scope = writer.function(validate_function, "static intptr_t", "uint8_t 
> *message_start, uint8_t *message_end, uint64_t offset, SPICE_GNUC_UNUSED int 
> minor")
> scope.variable_def("uint8_t *", "start = message_start + offset")
> scope.variable_def("SPICE_GNUC_UNUSED uint8_t *", "pos")
> -scope.variable_def("size_t", "mem_size", "nw_size")
> +scope.variable_def("uint64_t", "mem_size", "nw_size")
> num_pointers = struct.get_num_pointers()
> if  num_pointers != 0:
> scope.variable_def("SPICE_GNUC_UNUSED intptr_t", "ptr_size")
> @@ -236,7 +236,7 @@ def write_validate_struct_function(writer, struct):
> 
> writer.newline()
> writer.comment("Check if struct fits in reported side").newline()
> -writer.error_check("start + nw_size > message_end")
> +writer.error_check("nw_size > (uintptr_t) (message_end - start)")
> 
> writer.statement("return mem_size")
> 
> @@ -264,26 +264,26 @@ def write_validate_pointer_item(writer, container, 
> item, scope, parent_scope, st
> # if array, need no function check
> 
> if target_type.is_array():
> -writer.error_check("message_start + %s >= message_end" % v)
> +writer.error_check("%s >= (uintptr_t) (message_end - 
> message_start)" % v)
> 
> 
> assert target_type.element_type.is_primitive()
> 
> array_item = ItemInfo(target_type, "%s__array" % item.prefix, 
> start)
> -scope.variable_def("uint32_t", array_item.nw_size())
> +scope.variable_def("uint64_t", array_item.nw_size())
> # don't create a variable that isn't used, fixes 
> -Werror=unused-but-set-variable
> need_mem_size = want_mem_size or (
> want_extra_size and not item.member.has_attr("chunk")
> and not target_type.is_cstring_length())
> if need_mem_size:
> -scope.variable_def("uint32_t", array_item.mem_size())
> +scope.variable_def("uint64_t", array_item.mem_size())
> if target_type.is_cstring_length():
>

Re: [Spice-devel] [PATCH spice-server] stream-channel: Implements monitors_config

2018-03-20 Thread Christophe de Dinechin



> On 19 Mar 2018, at 11:39, Frediano Ziglio  wrote:
> 
>> 
>> On 03/13/2018 08:21 AM, Frediano Ziglio wrote:
>>> Although not necessary for a single monitor DisplayChannel implementation
>>> this make the DiisplayChannels more coherent from the client
>>> point of view.
>>> 
>>> Signed-off-by: Frediano Ziglio 
>>> ---
>>>  server/stream-channel.c | 33 ++---
>>>  1 file changed, 30 insertions(+), 3 deletions(-)
>>> 
>>> diff --git a/server/stream-channel.c b/server/stream-channel.c
>>> index 3a3b733f..a9882d9c 100644
>>> --- a/server/stream-channel.c
>>> +++ b/server/stream-channel.c
>>> @@ -102,6 +102,7 @@ enum {
>>>  RED_PIPE_ITEM_TYPE_STREAM_DATA,
>>>  RED_PIPE_ITEM_TYPE_STREAM_DESTROY,
>>>  RED_PIPE_ITEM_TYPE_STREAM_ACTIVATE_REPORT,
>>> +RED_PIPE_ITEM_TYPE_MONITORS_CONFIG,
>>>  };
>>> 
>>>  typedef struct StreamCreateItem {
>>> @@ -204,6 +205,26 @@ fill_base(SpiceMarshaller *m, const StreamChannel
>>> *channel)
>>>  spice_marshall_DisplayBase(m, );
>>>  }
>>> 
>>> +static void
>>> +marshall_monitors_config(RedChannelClient *rcc, StreamChannel *channel,
>>> SpiceMarshaller *m)
>>> +{
>>> +struct {
>>> +SpiceMsgDisplayMonitorsConfig config;
>>> +SpiceHead head;
>>> +} msg = {
>>> +{ 1, 1, },
>>> +{
>>> +0, PRIMARY_SURFACE_ID,
>> 
>> Hi Frediano,
>> 
>> Why do you send id=0 ?
>> 
>> Uri.
>> 
> 
> Too much IDs :-)
> These IDs are allocated starting from 0 and are per channel so as there
> is only a monitor (currently) the ID should be 0.
> So this id should not be confused with QXL ID or client/guest ID.

If Uri is confused, a comment may be in order.

Why so many IDs? Is it just to avoid an ID lookup during initialization?

> 
> Frediano
> 
>>> +channel->width, channel->height,
>>> +0, 0,
>>> +0 // flags
>>> +}
>>> +};
>>> +
>>> +red_channel_client_init_send_data(rcc,
>>> SPICE_MSG_DISPLAY_MONITORS_CONFIG);
>>> +spice_marshall_msg_display_monitors_config(m, );
>>> +}
>>> +
>>>  static void
>>>  stream_channel_send_item(RedChannelClient *rcc, RedPipeItem *pipe_item)
>>>  {
>>> @@ -229,6 +250,12 @@ stream_channel_send_item(RedChannelClient *rcc,
>>> RedPipeItem *pipe_item)
>>>  spice_marshall_msg_display_surface_create(m, _create);
>>>  break;
>>>  }
>>> +case RED_PIPE_ITEM_TYPE_MONITORS_CONFIG:
>>> +if (!red_channel_client_test_remote_cap(rcc,
>>> SPICE_DISPLAY_CAP_MONITORS_CONFIG)) {
>>> +return;
>>> +}
>>> +marshall_monitors_config(rcc, channel, m);
>>> +break;
>>>  case RED_PIPE_ITEM_TYPE_SURFACE_DESTROY: {
>>>  red_channel_client_init_send_data(rcc,
>>>  SPICE_MSG_DISPLAY_SURFACE_DESTROY);
>>>  SpiceMsgSurfaceDestroy surface_destroy = { PRIMARY_SURFACE_ID };
>>> @@ -397,7 +424,6 @@ stream_channel_connect(RedChannel *red_channel,
>>> RedClient *red_client, RedStream
>>>  request_new_stream(channel, start);
>>> 
>>> 
>>> -// TODO set capabilities like  SPICE_DISPLAY_CAP_MONITORS_CONFIG
>>>  // see guest_set_client_capabilities
>>>  RedChannelClient *rcc = RED_CHANNEL_CLIENT(client);
>>>  red_channel_client_push_set_ack(rcc);
>>> @@ -415,6 +441,7 @@ stream_channel_connect(RedChannel *red_channel,
>>> RedClient *red_client, RedStream
>>> 
>>>  // pass proper data
>>>  red_channel_client_pipe_add_type(rcc,
>>>  RED_PIPE_ITEM_TYPE_SURFACE_CREATE);
>>> +red_channel_client_pipe_add_type(rcc,
>>> RED_PIPE_ITEM_TYPE_MONITORS_CONFIG);
>>>  // surface data
>>>  red_channel_client_pipe_add_type(rcc,
>>>  RED_PIPE_ITEM_TYPE_FILL_SURFACE);
>>>  // TODO monitor configs ??
>>> @@ -433,8 +460,7 @@ stream_channel_constructed(GObject *object)
>>>  client_cbs.connect = stream_channel_connect;
>>>  red_channel_register_client_cbs(red_channel, _cbs, NULL);
>>> 
>>> -// TODO, send monitor to support multiple monitors in the future
>>> -//red_channel_set_cap(red_channel, SPICE_DISPLAY_CAP_MONITORS_CONFIG);
>>> +red_channel_set_cap(red_channel, SPICE_DISPLAY_CAP_MONITORS_CONFIG);
>>>  red_channel_set_cap(red_channel, SPICE_DISPLAY_CAP_STREAM_REPORT);
>>> 
>>>  reds_register_channel(reds, red_channel);
>>> @@ -489,6 +515,7 @@ stream_channel_change_format(StreamChannel *channel,
>>> const StreamMsgFormat *fmt)
>>>  channel->width = fmt->width;
>>>  channel->height = fmt->height;
>>>  red_channel_pipes_add_type(red_channel,
>>>  RED_PIPE_ITEM_TYPE_SURFACE_CREATE);
>>> +red_channel_pipes_add_type(red_channel,
>>> RED_PIPE_ITEM_TYPE_MONITORS_CONFIG);
>>>  // TODO monitors config ??
>>>  red_channel_pipes_add_empty_msg(red_channel,
>>>  SPICE_MSG_DISPLAY_MARK);
>>>  }
>>> 
>> 
>> 
> ___
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
>

Re: [Spice-devel] [PATCH spice-server 2/2] Use --enable-extra-checks option provided by spice-common

2018-03-20 Thread Christophe de Dinechin

See my comments on the spice-common patch.

> On 19 Mar 2018, at 14:46, Frediano Ziglio  wrote:
> 
> Reuse option from common code.
> Also reuse spice_extra_checks constant instead of using the preprocessor
> macro directly.
> 
> Signed-off-by: Frediano Ziglio 
> ---
> configure.ac   | 9 +
> server/display-channel.c   | 2 +-
> server/red-stream-device.c | 6 +++---
> server/reds.c  | 2 +-
> spice-common   | 2 +-
> 5 files changed, 7 insertions(+), 14 deletions(-)
> 
> diff --git a/configure.ac b/configure.ac
> index 86383434..2443ccf3 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -71,6 +71,7 @@ esac
> dnl =
> dnl Check optional features
> SPICE_CHECK_SMARTCARD
> +SPICE_EXTRA_CHECKS
> 
> AC_ARG_ENABLE(gstreamer,
>   
> AS_HELP_STRING([--enable-gstreamer=@<:@auto/0.10/1.0/yes/no@:>@],
> @@ -237,14 +238,6 @@ AC_ARG_ENABLE([statistics],
> AS_IF([test "$enable_statistics" = "yes"],
>   [AC_DEFINE([RED_STATISTICS], [1], [Enable SPICE statistics])])
> 
> -AC_ARG_ENABLE([extra-checks],
> -   AS_HELP_STRING([--enable-extra-checks=@<:@yes/no@:>@],
> -  [Enable expensive checks @<:@default=no@:>@]))
> -AM_CONDITIONAL(ENABLE_EXTRA_CHECKS, test "$enable_extra_checks" = "yes")
> -AC_DEFINE_UNQUOTED([ENABLE_EXTRA_CHECKS],
> -   [$(test "x$enable_extra_checks" = xyes && echo 1 || echo 
> 0)],
> -   [Define to 1 to enable extra checks on code otherwise 
> define to 0])
> -
> dnl 
> ===
> dnl check compiler flags
> 
> diff --git a/server/display-channel.c b/server/display-channel.c
> index 6dc10ee7..229f2c0f 100644
> --- a/server/display-channel.c
> +++ b/server/display-channel.c
> @@ -89,7 +89,7 @@ display_channel_finalize(GObject *object)
> display_channel_destroy_surfaces(self);
> image_cache_reset(>priv->image_cache);
> 
> -if (ENABLE_EXTRA_CHECKS) {
> +if (spice_extra_checks) {
> unsigned int count;
> _Drawable *drawable;
> VideoStream *stream;
> diff --git a/server/red-stream-device.c b/server/red-stream-device.c
> index c87c4899..e91df88d 100644
> --- a/server/red-stream-device.c
> +++ b/server/red-stream-device.c
> @@ -195,7 +195,7 @@ handle_msg_invalid(StreamDevice *dev, 
> SpiceCharDeviceInstance *sin, const char *
> {
> static const char default_error_msg[] = "Protocol error";
> 
> -if (ENABLE_EXTRA_CHECKS) {
> +if (spice_extra_checks) {
> spice_assert(dev->hdr_pos >= sizeof(StreamDevHeader));
> }
> 
> @@ -232,7 +232,7 @@ handle_msg_format(StreamDevice *dev, 
> SpiceCharDeviceInstance *sin)
> {
> SpiceCharDeviceInterface *sif = spice_char_device_get_interface(sin);
> 
> -if (ENABLE_EXTRA_CHECKS) {
> +if (spice_extra_checks) {
> spice_assert(dev->hdr_pos >= sizeof(StreamDevHeader));
> spice_assert(dev->hdr.type == STREAM_TYPE_FORMAT);
> }
> @@ -260,7 +260,7 @@ handle_msg_data(StreamDevice *dev, 
> SpiceCharDeviceInstance *sin)
> SpiceCharDeviceInterface *sif = spice_char_device_get_interface(sin);
> int n;
> 
> -if (ENABLE_EXTRA_CHECKS) {
> +if (spice_extra_checks) {
> spice_assert(dev->hdr_pos >= sizeof(StreamDevHeader));
> spice_assert(dev->hdr.type == STREAM_TYPE_DATA);
> }
> diff --git a/server/reds.c b/server/reds.c
> index 752bf7c2..998f2ffa 100644
> --- a/server/reds.c
> +++ b/server/reds.c
> @@ -4442,7 +4442,7 @@ red_char_device_vdi_port_finalize(GObject *object)
> dev->priv->current_read_buf = NULL;
> }
> g_free(dev->priv->mig_data);
> -if (ENABLE_EXTRA_CHECKS) {
> +if (spice_extra_checks) {
> spice_assert(dev->priv->num_read_buf == 0);
> }
> 
> diff --git a/spice-common b/spice-common
> index 8096b120..cc4a7f5c 16
> --- a/spice-common
> +++ b/spice-common
> @@ -1 +1 @@
> -Subproject commit 8096b1206bb266b8d0b80b3e4c0d36fc621d772d
> +Subproject commit cc4a7f5c7535fd5ed6756549bb918f1a54e9ea11
> -- 
> 2.14.3
> 
> ___
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel

___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH spice-common 1/2] Add --enable-extra-checks option

2018-03-20 Thread Christophe de Dinechin



> On 19 Mar 2018, at 14:46, Frediano Ziglio  wrote:
> 
> Allow to enable code to do additional or expensive checks.

“Allow to enable…” -> “Add configuration option enabling expensive checks”

> The option should be used by higher level libraries.
> By default the option is disabled.
> 
> Signed-off-by: Frediano Ziglio 
> ---
> common/log.h |  6 ++
> configure.ac |  1 +
> m4/spice-deps.m4 | 14 ++
> 3 files changed, 21 insertions(+)
> 
> diff --git a/common/log.h b/common/log.h
> index 06d48d2..e0fd34b 100644
> --- a/common/log.h
> +++ b/common/log.h
> @@ -93,6 +93,12 @@ void spice_log(GLogLevelFlags log_level,
> }   \
> } G_STMT_END
> 
> +#if ENABLE_EXTRA_CHECKS
> +enum { spice_extra_checks = 1 };

I would suggest renaming “extra_checks” to “expensive_checks” if that’s really 
the intent.
And then, I would avoid bracketing inexpensive checks (like simple asserts) 
with it, because otherwise it introduces confusion.

Alternatively, if we think that asserts are expensive, then all spice_assert 
checks should be disabled by this. But I’d rather not.

(I already suggested a spice_hot_assert for assertions in hot code as a better 
way to express the intent than an if around spice_assert).

> +#else
> +enum { spice_extra_checks = 0 };
> +#endif
> +
> SPICE_END_DECLS
> 
> #endif /* H_SPICE_LOG */
> diff --git a/configure.ac b/configure.ac
> index 3542161..3da85de 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -27,6 +27,7 @@ fi
> AM_PROG_CC_C_O
> 
> SPICE_CHECK_SYSDEPS
> +SPICE_EXTRA_CHECKS
> 
> AC_ARG_ENABLE([alignment-checks],
>   AS_HELP_STRING([--enable-alignment-checks],
> diff --git a/m4/spice-deps.m4 b/m4/spice-deps.m4
> index 68e3091..a6f4b7b 100644
> --- a/m4/spice-deps.m4
> +++ b/m4/spice-deps.m4
> @@ -23,6 +23,20 @@ AC_DEFUN([SPICE_PRINT_MESSAGES],[
> ])
> 
> 
> +# SPICE_EXTRA_CHECKS()
> +# 
> +# Check for --enable-extra-checks option
> +# 
> +AC_DEFUN([SPICE_EXTRA_CHECKS],[
> +AC_ARG_ENABLE([extra-checks],
> +   AS_HELP_STRING([--enable-extra-checks=@<:@yes/no@:>@],
> +  [Enable expensive checks @<:@default=no@:>@]))
> +AM_CONDITIONAL(ENABLE_EXTRA_CHECKS, test "x$enable_extra_checks" = "xyes")
> +AS_IF([test "x$enable_extra_checks" = "xyes"],
> +  [AC_DEFINE([ENABLE_EXTRA_CHECKS], 1, [Enable extra checks on code])])
> +])
> +
> +
> # SPICE_CHECK_SYSDEPS()
> # -
> # Checks for header files and library functions needed by spice-common.
> -- 
> 2.14.3
> 
> ___
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel

___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH spice-server 2/2] stream-device: Handle capabilities

2018-03-20 Thread Christophe de Dinechin


Looks good, with minor nits.

> On 19 Mar 2018, at 17:46, Frediano Ziglio  wrote:
> 
> Handle capabilities from guest device.
> Send capability to the guest when device is opened.
> Currently there's no capabilities set on the message sent.
> On the tests we need to discard the capability message before
> reading the error.
> 
> Signed-off-by: Frediano Ziglio 
> ---
> server/red-stream-device.c| 66 +--
> server/tests/test-stream-device.c | 22 +
> 2 files changed, 85 insertions(+), 3 deletions(-)
> 
> Changes since v1:
> - rebased on master (with minor fix due to rename).
> 
> diff --git a/server/red-stream-device.c b/server/red-stream-device.c
> index e91df88d..1732b888 100644
> --- a/server/red-stream-device.c
> +++ b/server/red-stream-device.c
> @@ -1,6 +1,6 @@
> /* spice-server character device to handle a video stream
> 
> -   Copyright (C) 2017 Red Hat, Inc.
> +   Copyright (C) 2017-2018 Red Hat, Inc.
> 
>This library is free software; you can redistribute it and/or
>modify it under the terms of the GNU Lesser General Public
> @@ -25,6 +25,8 @@
> #include "cursor-channel.h"
> #include "reds.h"
> 
> +#define MAX_GUEST_CAPABILITIES_BYTES ((STREAM_CAP_END+7)/8)
> +
> struct StreamDevice {
> RedCharDevice parent;
> 
> @@ -42,6 +44,7 @@ struct StreamDevice {
> bool has_error;
> bool opened;
> bool flow_stopped;
> +uint8_t guest_capabilities[MAX_GUEST_CAPABILITIES_BYTES];
> StreamChannel *stream_channel;
> CursorChannel *cursor_channel;
> SpiceTimer *close_timer;
> @@ -61,7 +64,7 @@ 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_cursor_move, handle_msg_capabilities;
> 
> static bool handle_msg_invalid(StreamDevice *dev, SpiceCharDeviceInstance 
> *sin,
>const char *error_msg) 
> SPICE_GNUC_WARN_UNUSED_RESULT;
> @@ -147,7 +150,8 @@ stream_device_partial_read(StreamDevice *dev, 
> SpiceCharDeviceInstance *sin)
> }
> break;
> case STREAM_TYPE_CAPABILITIES:
> -/* FIXME */
> +handled = handle_msg_capabilities(dev, sin);
> +break;
> default:
> handled = handle_msg_invalid(dev, sin, "Invalid message type");
> break;
> @@ -254,6 +258,38 @@ handle_msg_format(StreamDevice *dev, 
> SpiceCharDeviceInstance *sin)
> return true;
> }
> 
> +static bool
> +handle_msg_capabilities(StreamDevice *dev, SpiceCharDeviceInstance *sin)
> +{
> +SpiceCharDeviceInterface *sif = spice_char_device_get_interface(sin);
> +
> +if (spice_extra_checks) {

Premature optimization.

> +spice_assert(dev->hdr_pos >= sizeof(StreamDevHeader));
> +spice_assert(dev->hdr.type == STREAM_TYPE_CAPABILITIES);
> +}
> +
> +if (dev->hdr.size > STREAM_MSG_CAPABILITIES_MAX_BYTES) {
> +return handle_msg_invalid(dev, sin, "Wrong size for 
> StreamMsgCapabilities");
> +}
> +
> +int n = sif->read(sin, dev->msg->buf + dev->msg_pos, dev->hdr.size - 
> dev->msg_pos);
> +if (n < 0) {

Reading the various sif->read, the convention on return values is a bit 
unclear. Most other places seem to check for <= 0. Only handle_msg_format uses 
< 0. Someone could teach me why?

Is it possible for sif->read to return 0 on error?
Is it possible for sif->read to return less than the requested size (e.g. 
EINTR)?


> +return handle_msg_invalid(dev, sin, NULL);
> +}
> +
> +dev->msg_pos += n;
> +
> +if (dev->msg_pos < dev->hdr.size) {
> +return false;
> +}
> +
> +// copy only capabilities we care about

I think it’s “capabilities we know about”, since ifsd we get extra, it’s 
probably stuff added after this version.

> +memset(dev->guest_capabilities, 0, sizeof(dev->guest_capabilities));
> +memcpy(dev->guest_capabilities, dev->msg->buf, 
> MIN(MAX_GUEST_CAPABILITIES_BYTES, dev->hdr.size));
> +
> +return true;
> +}
> +
> static bool
> handle_msg_data(StreamDevice *dev, SpiceCharDeviceInstance *sin)
> {
> @@ -586,6 +622,28 @@ char_device_set_state(RedCharDevice *char_dev, int state)
> }
> }
> 
> +static void
> +send_capabilities(RedCharDevice *char_dev)
> +{
> +int msg_size = MAX_GUEST_CAPABILITIES_BYTES;
> +int total_size = sizeof(StreamDevHeader) + msg_size;
> +
> +RedCharDeviceWriteBuffer *buf =
> +red_char_device_write_buffer_get_server_no_token(char_dev, 
> total_size);
> +buf->buf_used = total_size;
> +
> +StreamDevHeader *const hdr = (StreamDevHeader *)buf->buf;
> +hdr->protocol_version = STREAM_DEVICE_PROTOCOL;
> +hdr->padding = 0;
> +hdr->type = GUINT16_TO_LE(STREAM_TYPE_CAPABILITIES);
> +hdr->size = GUINT32_TO_LE(msg_size);

We don’t have a macro/function taking care of filling a

Re: [Spice-devel] [PATCH spice-server 2/2] stream-device: Handle capabilities

Re: [Spice-devel] [PATCH spice-server 2/2] stream-device: Handle capabilities

[Spice-devel] [spice-vdagnet (linux) PATCH] x11-randr: do not assume each output has ncrtc=1

Re: [Spice-devel] [PATCH spice-server 2/2] stream-device: Handle capabilities

Re: [Spice-devel] [PATCH spice-server] stream-channel: Implements monitors_config

Re: [Spice-devel] [PATCH spice-common 1/2] Add --enable-extra-checks option

Re: [Spice-devel] [PATCH spice-common 3/3] Avoid integer overflow computing image sizes

Re: [Spice-devel] [PATCH spice-common 1/3] Fix integer overflows computing sizes

Re: [Spice-devel] [PATCH spice-common 3/3] Avoid integer overflow computing image sizes

Re: [Spice-devel] [PATCH spice-common 1/3] Fix integer overflows computing sizes

Re: [Spice-devel] [PATCH spice-server] stream-channel: Implements monitors_config

Re: [Spice-devel] [PATCH spice-server 2/2] Use --enable-extra-checks option provided by spice-common

Re: [Spice-devel] [PATCH spice-common 1/2] Add --enable-extra-checks option

Re: [Spice-devel] [PATCH spice-server 2/2] stream-device: Handle capabilities

14 matches

Site Navigation

Mail list logo

Footer information