>
> On 03/20/2018 01:28 PM, Frediano Ziglio wrote:
> >>
> >> Looks good, with minor nits.
> >>
> >>> On 19 Mar 2018, at 17:46, Frediano Ziglio <fziglio@xxxxxxxxxx> 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 <fziglio@xxxxxxxxxx>
> >>> ---
> >>> 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
>
Yes
> >>> +
> >>> 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
>
No, you are assuming the client is not sending capabilities which is
wrong, client should be free to send whatever it wants, but yes,
currently will be 0.
> The call sif->read(sin,buffer,0) returns 0.
>
Yes, as standard Unix read does and is correctly supported.
> Uri
Frediano
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel