> On 20 Mar 2018, at 12:28, Frediano Ziglio <fziglio@xxxxxxxxxx> 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)
>>> +
>>> 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) {
As an aside, the fact that you used a lower-case for an enum in violation of the existing practice in the code makes this look like a regular test, not a configuration test.
>>
>> Premature optimization.
>>
>
> Extra is not expensive
If it’s not expensive, then what is it? (See my comments in other patch, the alternatives to “expensive” in english are “superfluous” or “superior”).
> and code is coherent with other part.
Yes, I’m well aware you consistently ignored my input on this topic earlier :-)
To be clear, I”m nacking the “if”, not the assert, in
if (spice_extra_check)
{
spice_assert(dev->hdr_pos >= sizeof(StreamDevHeader));
spice_assert(dev->hdr.type == STREAM_TYPE_CAPABILITIES);
}
The “if” means “these tests have something special that makes them worth skipping by default”. Therefore, the “if” test is basically lying to me :-) which is why I want it gone.
A wise man once wrote on this list that he would rather have a core than a remote execution. I believe that you should listen to him.
>>> + }
>
>>> + 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.
That’s what I guessed. Thanks for confirming.
> Basically with <=0 you handle either 0 bytes or error while with <0 only
> errors.
So here, if n < dev->hdr.size - dev->msg.pos case, we fall through and we retry later because we return false. Got it.
>
>> 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.
This logic is repeated many times. A bit WET to my taste. But OK, at least now I understand.
>
>>
>>> + 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 @@ 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 StreamDevHeader?
>>
>
> No, there are not much messages for the guest, can be introduced
Should be introduced according to DRY principle.
>
>>> +
>>> + 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);
>>> }
>>> dev->hdr_pos = 0;
>>> dev->msg_pos = 0;
>>> diff --git a/server/tests/test-stream-device.c
>>> b/server/tests/test-stream-device.c
>>> index 3c9209a4..43011f9d 100644
>>> --- a/server/tests/test-stream-device.c
>>> +++ b/server/tests/test-stream-device.c
>>> @@ -135,12 +135,33 @@ static uint8_t *add_format(uint8_t *p, uint32_t w,
>>> uint32_t h, SpiceVideoCodecTy
>>> return p + sizeof(fmt);
>>> }
>>>
>>> +// remove capabilities from server reply
>>
>> The name already says that (except it uses “consume” and not “remove”.
>> The comment would be more helpful to me if it explained why this is
>> necessary.
>> Based on the fact this is to make a test pass, I think that
>> “skip_server_capabilities” or “ignore_server_capbilities” would be a better
>> name.
>>
>
> "consume" is used in the demarshalling code, I think is more appropriate
> as indicate that data is removed from the buffer.
>
> Yes, comment does not add much, I'll remove
Or you could replace it with “capabilities are not tested at the moment, we ignore them” or something like that.
>
>>> +static void
>>> +consume_server_capabilities(void)
>>> +{
>>> + StreamDevHeader hdr;
>>> +
>>> + if (vmc_write_pos == 0) {
>>> + return;
>>> + }
>>> + g_assert(vmc_write_pos >= sizeof(hdr));
>>> +
>>> + memcpy(&hdr, vmc_write_buf, sizeof(hdr));
>>> + if (GUINT16_FROM_LE(hdr.type) == STREAM_TYPE_CAPABILITIES) {
>>> + g_assert_cmpint(GUINT32_FROM_LE(hdr.size), <=, vmc_write_pos -
>>> sizeof(hdr));
>>> + vmc_write_pos -= GUINT32_FROM_LE(hdr.size) + sizeof(hdr);
>>> + memmove(vmc_write_buf, vmc_write_buf + GUINT32_FROM_LE(hdr.size) +
>>> sizeof(hdr), vmc_write_pos);
>>> + }
>>> +}
>>> +
>>> // check we have an error message on the write buffer
>>> static void
>>> check_vmc_error_message(void)
>>> {
>>> StreamDevHeader hdr;
>>>
>>> + consume_server_capabilities();
>>> +
>>> g_assert(vmc_write_pos >= sizeof(hdr));
>>>
>>> memcpy(&hdr, vmc_write_buf, sizeof(hdr));
>>> @@ -245,6 +266,7 @@ static void test_stream_device_unfinished(void)
>>> g_assert(message_sizes_curr - message_sizes == 1);
>>>
>>> // we should have no data from the device
>>> + consume_server_capabilities();
>>> g_assert_cmpint(vmc_write_pos, ==, 0);
>>>
>>> test_destroy(test);
>
> Frediano
> _______________________________________________
> Spice-devel mailing list
> Spice-devel@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel