> On 21 Mar 2018, at 18:07, Frediano Ziglio <fziglio@xxxxxxxxxx> wrote:
>
>>>
>>> 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.
>>
>
> You are right, maybe upper case would be better.
>
>>>>
>>>> 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.
>>
>
> In this case the reason is "paranoia". The size test is a duplicate,
> the type is like a malloc function that is checking if the caller
> really wants to allocate memory.
I really believe that it is a bad habit to disable asserts locally on a case-by-case basis. It makes code really hard to read and unnecessarily puzzling. If I have to scratch my head looking at this and wait until you explain that the extra if is because you think the caller already validated that, the if is unhelpful.
That being said, I am very much in favor of finer-grained assert classes, that actually document your intent, and already suggested something like that:
// Asserts in hot code that we don’t want to check in production code
#define spice_hot_assert(x) spice_assert(SPICE_EXTRA_CHECKS && (x))
// The following are asserts for developers only, disabled in production code
// A condition that must be true when entering a function
#define spice_precondition(x) spice_assert(SPICE_EXTRA_CHECK && (x))
// A condition that must be true when exiting a function
#define spice_postcondition(x) spice_assert(SPICE_EXTRA_CHECK && (x))
// A condition that must be true continuously, e.g. within a loop
#define spice_invariant(x) spice_assert(SPICE_EXTRA_CHECK && (x))
Then you could write your assert as “spice_precondition(…)” and we’d both be happy.
Leave your code as is for now, now that I understand your rationale. I will submit a patch along the lines of the above.
As an aside, in case you respin that patch, there is a habit that I saw several other projects doing, which is something like this:
+ spice_assert(dev->hdr_pos >= sizeof(StreamDevHeader) && “Caller supplied a bad header position");
That way, when an assert fail, you get some additional “documentation” on the printout, and no extra cost since the text constant is known to be always true by the compiler.
>
>> 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.
>>
>
> These tests do not prevent a remote execution. These kind of test are
> more used during testing if you really screw up things.
Understood (now).
>
>>
>>
>>>>> + }
>>>
>>>>> + 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.
>>
>
> I think a patch in server/spice-char.h adding some comments would be
> great. Whoever wrote these interfaces didn't do it leaving the implementer
> to guess or having to look at the implementation for these details which
> are far from easy to grasp. A read that by standard is not blocking or
> behave differently based on the state of the device is far from what you
> expect.
>
>>> 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.
>>
>
> Yes, I'm realizing I did a bit of too much copy and paste too.
>
>>>
>>>>
>>>>> + 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.
>>
>
> I'll add one as a preparation patch.
>
>>>
>>>>> +
>>>>> + 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.
>>
>
> Looks like more a comment for the caller or a use case.
> I'll try to add.
>
>>>
>>>>> +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