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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Following the explanations,

Acked-by: Christophe de Dinechin <dinechin@xxxxxxxxxx> 

(“preparation” patch, uppercase, etc can all be followups)


> 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.
> 
>> 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.
> 
>> 
>> 
>>>>> +    }
>>> 
>>>>> +        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




[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux]     [Linux OMAP]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]     [Monitors]